Re: [PATCH V3 5/8] dt-bindings: Add xen,grant-dma IOMMU description for xen-grant DMA ops
On Tue, 31 May 2022, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko
>
> The main purpose of this binding is to communicate Xen specific
> information using generic IOMMU device tree bindings (which is
> a good fit here) rather than introducing a custom property.
>
> Introduce Xen specific IOMMU for the virtualized device (e.g. virtio)
> to be used by Xen grant DMA-mapping layer in the subsequent commit.
>
> The reference to Xen specific IOMMU node using "iommus" property
> indicates that Xen grant mappings need to be enabled for the device,
> and it specifies the ID of the domain where the corresponding backend
> resides. The domid (domain ID) is used as an argument to the Xen grant
> mapping APIs.
>
> This is needed for the option to restrict memory access using Xen grant
> mappings to work which primary goal is to enable using virtio devices
> in Xen guests.
>
> Signed-off-by: Oleksandr Tyshchenko
> ---
> Changes RFC -> V1:
>- update commit subject/description and text in description
>- move to devicetree/bindings/arm/
>
> Changes V1 -> V2:
>- update text in description
>- change the maintainer of the binding
>- fix validation issue
>- reference xen,dev-domid.yaml schema from virtio/mmio.yaml
>
> Change V2 -> V3:
>- Stefano already gave his Reviewed-by, I dropped it due to the changes
> (significant)
>- use generic IOMMU device tree bindings instead of custom property
> "xen,dev-domid"
>- change commit subject and description, was
> "dt-bindings: Add xen,dev-domid property description for xen-grant DMA
> ops"
> ---
> .../devicetree/bindings/iommu/xen,grant-dma.yaml | 49
> ++
> 1 file changed, 49 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iommu/xen,grant-dma.yaml
>
> diff --git a/Documentation/devicetree/bindings/iommu/xen,grant-dma.yaml
> b/Documentation/devicetree/bindings/iommu/xen,grant-dma.yaml
> new file mode 100644
> index ..ab5765c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iommu/xen,grant-dma.yaml
> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iommu/xen,grant-dma.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Xen specific IOMMU for virtualized devices (e.g. virtio)
> +
> +maintainers:
> + - Stefano Stabellini
> +
> +description:
> + The reference to Xen specific IOMMU node using "iommus" property indicates
> + that Xen grant mappings need to be enabled for the device, and it specifies
> + the ID of the domain where the corresponding backend resides.
> + The binding is required to restrict memory access using Xen grant mappings.
I think this is OK and in line with the discussion we had on the list. I
propose the following wording instead:
"""
The Xen IOMMU represents the Xen grant table interface. Grant mappings
are to be used with devices connected to the Xen IOMMU using the
"iommus" property, which also specifies the ID of the backend domain.
The binding is required to restrict memory access using Xen grant
mappings.
"""
> +properties:
> + compatible:
> +const: xen,grant-dma
> +
> + '#iommu-cells':
> +const: 1
> +description:
> + Xen specific IOMMU is multiple-master IOMMU device.
> + The single cell describes the domid (domain ID) of the domain where
> + the backend is running.
Here I would say:
"""
The single cell is the domid (domain ID) of the domain where the backend
is running.
"""
With the two wording improvements:
Reviewed-by: Stefano Stabellini
> +required:
> + - compatible
> + - "#iommu-cells"
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> +xen_iommu {
> +compatible = "xen,grant-dma";
> +#iommu-cells = <1>;
> +};
> +
> +virtio@3000 {
> +compatible = "virtio,mmio";
> +reg = <0x3000 0x100>;
> +interrupts = <41>;
> +
> +/* The backend is located in Xen domain with ID 1 */
> +iommus = <_iommu 1>;
> +};
> --
> 2.7.4
>
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/3] swiotlb: use the right nslabs-derived sizes in swiotlb_init_late
On Wed, 11 May 2022, Christoph Hellwig wrote:
> nslabs can shrink when allocations or the remap don't succeed, so make
> sure to use it for all sizing. For that remove the bytes value that
> can get stale and replace it with local calculations and a boolean to
> indicate if the originally requested size could not be allocated.
>
> Fixes: 6424e31b1c05 ("swiotlb: remove swiotlb_init_with_tbl and
> swiotlb_init_late_with_tbl")
> Signed-off-by: Christoph Hellwig
Reviewed-by: Stefano Stabellini
> ---
> kernel/dma/swiotlb.c | 19 +++
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index 113e1e8aaca37..d6e62a6a42ceb 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -297,9 +297,9 @@ int swiotlb_init_late(size_t size, gfp_t gfp_mask,
> {
> struct io_tlb_mem *mem = _tlb_default_mem;
> unsigned long nslabs = ALIGN(size >> IO_TLB_SHIFT, IO_TLB_SEGSIZE);
> - unsigned long bytes;
> unsigned char *vstart = NULL;
> unsigned int order;
> + bool retried = false;
> int rc = 0;
>
> if (swiotlb_force_disable)
> @@ -308,7 +308,6 @@ int swiotlb_init_late(size_t size, gfp_t gfp_mask,
> retry:
> order = get_order(nslabs << IO_TLB_SHIFT);
> nslabs = SLABS_PER_PAGE << order;
> - bytes = nslabs << IO_TLB_SHIFT;
>
> while ((SLABS_PER_PAGE << order) > IO_TLB_MIN_SLABS) {
> vstart = (void *)__get_free_pages(gfp_mask | __GFP_NOWARN,
> @@ -316,16 +315,13 @@ int swiotlb_init_late(size_t size, gfp_t gfp_mask,
> if (vstart)
> break;
> order--;
> + nslabs = SLABS_PER_PAGE << order;
> + retried = true;
> }
>
> if (!vstart)
> return -ENOMEM;
>
> - if (order != get_order(bytes)) {
> - pr_warn("only able to allocate %ld MB\n",
> - (PAGE_SIZE << order) >> 20);
> - nslabs = SLABS_PER_PAGE << order;
> - }
> if (remap)
> rc = remap(vstart, nslabs);
> if (rc) {
> @@ -334,9 +330,15 @@ int swiotlb_init_late(size_t size, gfp_t gfp_mask,
> nslabs = ALIGN(nslabs >> 1, IO_TLB_SEGSIZE);
> if (nslabs < IO_TLB_MIN_SLABS)
> return rc;
> + retried = true;
> goto retry;
> }
>
> + if (retried) {
> + pr_warn("only able to allocate %ld MB\n",
> + (PAGE_SIZE << order) >> 20);
> + }
> +
> mem->slots = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
> get_order(array_size(sizeof(*mem->slots), nslabs)));
> if (!mem->slots) {
> @@ -344,7 +346,8 @@ int swiotlb_init_late(size_t size, gfp_t gfp_mask,
> return -ENOMEM;
> }
>
> - set_memory_decrypted((unsigned long)vstart, bytes >> PAGE_SHIFT);
> + set_memory_decrypted((unsigned long)vstart,
> + (nslabs << IO_TLB_SHIFT) >> PAGE_SHIFT);
> swiotlb_init_io_tlb_mem(mem, virt_to_phys(vstart), nslabs, true);
>
> swiotlb_print_info();
> --
> 2.30.2
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/3] swiotlb: use the right nslabs value in swiotlb_init_remap
On Wed, 11 May 2022, Christoph Hellwig wrote:
> default_nslabs should only be used to initialize nslabs, after that we
> need to use the local variable that can shrink when allocations or the
> remap don't succeed.
>
> Fixes: 6424e31b1c05 ("swiotlb: remove swiotlb_init_with_tbl and
> swiotlb_init_late_with_tbl")
> Signed-off-by: Christoph Hellwig
Reviewed-by: Stefano Stabellini
> ---
> kernel/dma/swiotlb.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index 3e992a308c8a1..113e1e8aaca37 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -234,7 +234,7 @@ void __init swiotlb_init_remap(bool addressing_limit,
> unsigned int flags,
> {
> struct io_tlb_mem *mem = _tlb_default_mem;
> unsigned long nslabs = default_nslabs;
> - size_t alloc_size = PAGE_ALIGN(array_size(sizeof(*mem->slots), nslabs));
> + size_t alloc_size;
> size_t bytes;
> void *tlb;
>
> @@ -249,7 +249,7 @@ void __init swiotlb_init_remap(bool addressing_limit,
> unsigned int flags,
>* memory encryption.
>*/
> retry:
> - bytes = PAGE_ALIGN(default_nslabs << IO_TLB_SHIFT);
> + bytes = PAGE_ALIGN(nslabs << IO_TLB_SHIFT);
> if (flags & SWIOTLB_ANY)
> tlb = memblock_alloc(bytes, PAGE_SIZE);
> else
> @@ -269,12 +269,13 @@ void __init swiotlb_init_remap(bool addressing_limit,
> unsigned int flags,
> goto retry;
> }
>
> + alloc_size = PAGE_ALIGN(array_size(sizeof(*mem->slots), nslabs));
> mem->slots = memblock_alloc(alloc_size, PAGE_SIZE);
> if (!mem->slots)
> panic("%s: Failed to allocate %zu bytes align=0x%lx\n",
> __func__, alloc_size, PAGE_SIZE);
>
> - swiotlb_init_io_tlb_mem(mem, __pa(tlb), default_nslabs, false);
> + swiotlb_init_io_tlb_mem(mem, __pa(tlb), nslabs, false);
> mem->force_bounce = flags & SWIOTLB_FORCE;
>
> if (flags & SWIOTLB_VERBOSE)
> --
> 2.30.2
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/3] swiotlb: don't panic when the swiotlb buffer can't be allocated
On Wed, 11 May 2022, Christoph Hellwig wrote:
> For historical reasons the switlb code paniced when the metadata could
> not be allocated, but just printed a warning when the actual main
> swiotlb buffer could not be allocated. Restore this somewhat unexpected
> behavior as changing it caused a boot failure on the Microchip RISC-V
> PolarFire SoC Icicle kit.
>
> Fixes: 6424e31b1c05 ("swiotlb: remove swiotlb_init_with_tbl and
> swiotlb_init_late_with_tbl")
> Reported-by: Conor Dooley
> Signed-off-by: Christoph Hellwig
> Tested-by: Conor Dooley
Reviewed-by: Stefano Stabellini
> ---
> kernel/dma/swiotlb.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index e2ef0864eb1e5..3e992a308c8a1 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -254,8 +254,10 @@ void __init swiotlb_init_remap(bool addressing_limit,
> unsigned int flags,
> tlb = memblock_alloc(bytes, PAGE_SIZE);
> else
> tlb = memblock_alloc_low(bytes, PAGE_SIZE);
> - if (!tlb)
> - panic("%s: failed to allocate tlb structure\n", __func__);
> + if (!tlb) {
> + pr_warn("%s: failed to allocate tlb structure\n", __func__);
> + return;
> + }
>
> if (remap && remap(tlb, nslabs) < 0) {
> memblock_free(tlb, PAGE_ALIGN(bytes));
> --
> 2.30.2
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] swiotlb-xen: fix DMA_ATTR_NO_KERNEL_MAPPING on arm
On Wed, 11 May 2022, Christoph Hellwig wrote: > On Fri, Apr 29, 2022 at 04:15:38PM -0700, Stefano Stabellini wrote: > > Great! Christoph you can go ahead and pick it up in your tree if you are > > up for it. > > The patch is in the dma-mapping for-next brancch now: > > http://git.infradead.org/users/hch/dma-mapping.git/commitdiff/62cb1ca1654b57589c582efae2748159c74ee356 > > There were a few smaller merge conflicts with the swiotlb refactoring. > I think everything is fine, but please take another look if possible. Looks good to me ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] swiotlb-xen: fix DMA_ATTR_NO_KERNEL_MAPPING on arm
On Fri, 29 Apr 2022, Boris Ostrovsky wrote: > On 4/28/22 6:49 PM, Stefano Stabellini wrote: > > On Thu, 28 Apr 2022, Boris Ostrovsky wrote: > > > On 4/28/22 5:49 PM, Stefano Stabellini wrote: > > > > On Thu, 28 Apr 2022, Christoph Hellwig wrote: > > > > > On Tue, Apr 26, 2022 at 04:07:45PM -0700, Stefano Stabellini wrote: > > > > > > > Reported-by: Rahul Singh > > > > > > > Signed-off-by: Christoph Hellwig > > > > > > Reviewed-by: Stefano Stabellini > > > > > Do you want to take this through the Xen tree or should I pick it up? > > > > > Either way I'd love to see some testing on x86 as well. > > > > I agree on the x86 testing. Juergen, Boris? > > > > > > > > I'd say to take this patch via the Xen tree but Juergen has just sent a > > > > Xen pull request to Linux last Saturday. Juergen do you plan to send > > > > another one? Do you have something else lined up? If not, it might be > > > > better to let Christoph pick it up. > > > > > > We don't have anything pending. > > > > > > > > > I can test it but at best tomorrow so not sure we can get this into rc5. > > > Do > > > you consider this an urgent fix or can we wait until 5.19? Because it's a > > > bit > > > too much IMO for rc6. > > On one hand, Linux doesn't boot on a platform without this fix. On the > > other hand, I totally see that this patch could introduce regressions on > > x86 so I think it is fair that we are careful with it. > > > > From my point of view, it might be better to wait for 5.19 and mark it > > as backport. > > > No problems uncovered during testing. Great! Christoph you can go ahead and pick it up in your tree if you are up for it. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] swiotlb-xen: fix DMA_ATTR_NO_KERNEL_MAPPING on arm
On Thu, 28 Apr 2022, Boris Ostrovsky wrote: > On 4/28/22 5:49 PM, Stefano Stabellini wrote: > > On Thu, 28 Apr 2022, Christoph Hellwig wrote: > > > On Tue, Apr 26, 2022 at 04:07:45PM -0700, Stefano Stabellini wrote: > > > > > Reported-by: Rahul Singh > > > > > Signed-off-by: Christoph Hellwig > > > > Reviewed-by: Stefano Stabellini > > > Do you want to take this through the Xen tree or should I pick it up? > > > Either way I'd love to see some testing on x86 as well. > > I agree on the x86 testing. Juergen, Boris? > > > > I'd say to take this patch via the Xen tree but Juergen has just sent a > > Xen pull request to Linux last Saturday. Juergen do you plan to send > > another one? Do you have something else lined up? If not, it might be > > better to let Christoph pick it up. > > > We don't have anything pending. > > > I can test it but at best tomorrow so not sure we can get this into rc5. Do > you consider this an urgent fix or can we wait until 5.19? Because it's a bit > too much IMO for rc6. On one hand, Linux doesn't boot on a platform without this fix. On the other hand, I totally see that this patch could introduce regressions on x86 so I think it is fair that we are careful with it. >From my point of view, it might be better to wait for 5.19 and mark it as backport. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] swiotlb-xen: fix DMA_ATTR_NO_KERNEL_MAPPING on arm
On Thu, 28 Apr 2022, Christoph Hellwig wrote: > On Tue, Apr 26, 2022 at 04:07:45PM -0700, Stefano Stabellini wrote: > > > Reported-by: Rahul Singh > > > Signed-off-by: Christoph Hellwig > > > > Reviewed-by: Stefano Stabellini > > Do you want to take this through the Xen tree or should I pick it up? > Either way I'd love to see some testing on x86 as well. I agree on the x86 testing. Juergen, Boris? I'd say to take this patch via the Xen tree but Juergen has just sent a Xen pull request to Linux last Saturday. Juergen do you plan to send another one? Do you have something else lined up? If not, it might be better to let Christoph pick it up. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] swiotlb-xen: fix DMA_ATTR_NO_KERNEL_MAPPING on arm
On Sat, 23 Apr 2022, Christoph Hellwig wrote:
> swiotlb-xen uses very different ways to allocate coherent memory on x86
> vs arm. On the former it allocates memory from the page allocator, while
> on the later it reuses the dma-direct allocator the handles the
> complexities of non-coherent DMA on arm platforms.
>
> Unfortunately the complexities of trying to deal with the two cases in
> the swiotlb-xen.c code lead to a bug in the handling of
> DMA_ATTR_NO_KERNEL_MAPPING on arm. With the DMA_ATTR_NO_KERNEL_MAPPING
> flag the coherent memory allocator does not actually allocate coherent
> memory, but just a DMA handle for some memory that is DMA addressable
> by the device, but which does not have to have a kernel mapping. Thus
> dereferencing the return value will lead to kernel crashed and memory
> corruption.
>
> Fix this by using the dma-direct allocator directly for arm, which works
> perfectly fine because on arm swiotlb-xen is only used when the domain is
> 1:1 mapped, and then simplifying the remaining code to only cater for the
> x86 case with DMA coherent device.
>
> Reported-by: Rahul Singh
> Signed-off-by: Christoph Hellwig
Reviewed-by: Stefano Stabellini
> ---
> arch/arm/include/asm/xen/page-coherent.h | 2 -
> arch/arm/xen/mm.c | 17
> arch/arm64/include/asm/xen/page-coherent.h | 2 -
> arch/x86/include/asm/xen/page-coherent.h | 24 -
> arch/x86/include/asm/xen/swiotlb-xen.h | 5 +
> drivers/xen/swiotlb-xen.c | 106 -
> include/xen/arm/page-coherent.h| 20
> include/xen/xen-ops.h | 7 --
> 8 files changed, 45 insertions(+), 138 deletions(-)
> delete mode 100644 arch/arm/include/asm/xen/page-coherent.h
> delete mode 100644 arch/arm64/include/asm/xen/page-coherent.h
> delete mode 100644 arch/x86/include/asm/xen/page-coherent.h
> delete mode 100644 include/xen/arm/page-coherent.h
>
> diff --git a/arch/arm/include/asm/xen/page-coherent.h
> b/arch/arm/include/asm/xen/page-coherent.h
> deleted file mode 100644
> index 27e984977402b..0
> --- a/arch/arm/include/asm/xen/page-coherent.h
> +++ /dev/null
> @@ -1,2 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> -#include
> diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
> index a7e54a087b802..6e603e5fdebb1 100644
> --- a/arch/arm/xen/mm.c
> +++ b/arch/arm/xen/mm.c
> @@ -118,23 +118,6 @@ bool xen_arch_need_swiotlb(struct device *dev,
> !dev_is_dma_coherent(dev));
> }
>
> -int xen_create_contiguous_region(phys_addr_t pstart, unsigned int order,
> - unsigned int address_bits,
> - dma_addr_t *dma_handle)
> -{
> - if (!xen_initial_domain())
> - return -EINVAL;
> -
> - /* we assume that dom0 is mapped 1:1 for now */
> - *dma_handle = pstart;
> - return 0;
> -}
> -
> -void xen_destroy_contiguous_region(phys_addr_t pstart, unsigned int order)
> -{
> - return;
> -}
> -
> static int __init xen_mm_init(void)
> {
> struct gnttab_cache_flush cflush;
> diff --git a/arch/arm64/include/asm/xen/page-coherent.h
> b/arch/arm64/include/asm/xen/page-coherent.h
> deleted file mode 100644
> index 27e984977402b..0
> --- a/arch/arm64/include/asm/xen/page-coherent.h
> +++ /dev/null
> @@ -1,2 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> -#include
> diff --git a/arch/x86/include/asm/xen/page-coherent.h
> b/arch/x86/include/asm/xen/page-coherent.h
> deleted file mode 100644
> index 63cd41b2e17ac..0
> --- a/arch/x86/include/asm/xen/page-coherent.h
> +++ /dev/null
> @@ -1,24 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> -#ifndef _ASM_X86_XEN_PAGE_COHERENT_H
> -#define _ASM_X86_XEN_PAGE_COHERENT_H
> -
> -#include
> -#include
> -
> -static inline void *xen_alloc_coherent_pages(struct device *hwdev, size_t
> size,
> - dma_addr_t *dma_handle, gfp_t flags,
> - unsigned long attrs)
> -{
> - void *vstart = (void*)__get_free_pages(flags, get_order(size));
> - *dma_handle = virt_to_phys(vstart);
> - return vstart;
> -}
> -
> -static inline void xen_free_coherent_pages(struct device *hwdev, size_t size,
> - void *cpu_addr, dma_addr_t dma_handle,
> - unsigned long attrs)
> -{
> - free_pages((unsigned long) cpu_addr, get_order(size));
> -}
> -
> -#endif /* _ASM_X86_XEN_PAGE_COHERENT_H */
> diff --git a/arch/x86/include/asm/xen/swiotlb-xen.h
> b/arch/x86/include/asm/xen/swiotlb-xen.h
> index 66b4ddde77430..558821387808e 100644
> --- a/arch/x86/includ
Re: [PATCH 13/15] swiotlb: merge swiotlb-xen initialization into swiotlb
On Mon, 14 Mar 2022, Christoph Hellwig wrote: > Reuse the generic swiotlb initialization for xen-swiotlb. For ARM/ARM64 > this works trivially, while for x86 xen_swiotlb_fixup needs to be passed > as the remap argument to swiotlb_init_remap/swiotlb_init_late. > > Signed-off-by: Christoph Hellwig For arch/arm/xen and drivers/xen/swiotlb-xen.c Reviewed-by: Stefano Stabellini ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 11/12] swiotlb: merge swiotlb-xen initialization into swiotlb
On Fri, 4 Mar 2022, Christoph Hellwig wrote:
> On Thu, Mar 03, 2022 at 02:49:29PM -0800, Stefano Stabellini wrote:
> > On Thu, 3 Mar 2022, Christoph Hellwig wrote:
> > > On Wed, Mar 02, 2022 at 05:25:10PM -0800, Stefano Stabellini wrote:
> > > > Thinking more about it we actually need to drop the xen_initial_domain()
> > > > check otherwise some cases won't be functional (Dom0 not 1:1 mapped, or
> > > > DomU 1:1 mapped).
> > >
> > > Hmm, but that would be the case even before this series, right?
> >
> > Before this series we only have the xen_swiotlb_detect() check in
> > xen_mm_init, we don't have a second xen_initial_domain() check.
> >
> > The issue is that this series is adding one more xen_initial_domain()
> > check in xen_mm_init.
>
> In current mainline xen_mm_init calls xen_swiotlb_init unconditionally.
> But xen_swiotlb_init then calls xen_swiotlb_fixup after allocating
> the memory, which in turn calls xen_create_contiguous_region.
> xen_create_contiguous_region fails with -EINVAL for the
> !xen_initial_domain() and thus caues xen_swiotlb_fixup and
> xen_swiotlb_init to unwind and return -EINVAL.
>
> So as far as I can tell there is no change in behavior, but maybe I'm
> missing something subtle?
You are right.
The xen_initial_domain() check in xen_create_contiguous_region() is
wrong and we should get rid of it. It is a leftover from before the
xen_swiotlb_detect rework.
We could either remove it or change it into another xen_swiotlb_detect()
check.
Feel free to add the patch to your series or fold it with another patch
or rework it as you prefer. Thanks for spotting this!
---
arm/xen: don't check for xen_initial_domain() in xen_create_contiguous_region
It used to be that Linux enabled swiotlb-xen when running a dom0 on ARM.
Since f5079a9a2a31 "xen/arm: introduce XENFEAT_direct_mapped and
XENFEAT_not_direct_mapped", Linux detects whether to enable or disable
swiotlb-xen based on the new feature flags: XENFEAT_direct_mapped and
XENFEAT_not_direct_mapped.
However, there is still a leftover xen_initial_domain() check in
xen_create_contiguous_region. Remove the check as
xen_create_contiguous_region is only called by swiotlb-xen during
initialization. If xen_create_contiguous_region is called, we know Linux
is running 1:1 mapped so there is no need for additional checks.
Also update the in-code comment.
Signed-off-by: Stefano Stabellini
diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
index a7e54a087b80..28c207060253 100644
--- a/arch/arm/xen/mm.c
+++ b/arch/arm/xen/mm.c
@@ -122,10 +122,7 @@ int xen_create_contiguous_region(phys_addr_t pstart,
unsigned int order,
unsigned int address_bits,
dma_addr_t *dma_handle)
{
- if (!xen_initial_domain())
- return -EINVAL;
-
- /* we assume that dom0 is mapped 1:1 for now */
+ /* the domain is 1:1 mapped to use swiotlb-xen */
*dma_handle = pstart;
return 0;
}
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 11/12] swiotlb: merge swiotlb-xen initialization into swiotlb
On Thu, 3 Mar 2022, Christoph Hellwig wrote: > On Wed, Mar 02, 2022 at 05:25:10PM -0800, Stefano Stabellini wrote: > > Thinking more about it we actually need to drop the xen_initial_domain() > > check otherwise some cases won't be functional (Dom0 not 1:1 mapped, or > > DomU 1:1 mapped). > > Hmm, but that would be the case even before this series, right? Before this series we only have the xen_swiotlb_detect() check in xen_mm_init, we don't have a second xen_initial_domain() check. The issue is that this series is adding one more xen_initial_domain() check in xen_mm_init. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 11/12] swiotlb: merge swiotlb-xen initialization into swiotlb
On Wed, 2 Mar 2022, Christoph Hellwig wrote:
> On Tue, Mar 01, 2022 at 06:55:47PM -0800, Stefano Stabellini wrote:
> > Unrelated to this specific patch series: now that I think about it, if
> > io_tlb_default_mem.nslabs is already allocated by the time xen_mm_init
> > is called, wouldn't we potentially have an issue with the GFP flags used
> > for the earlier allocation (e.g. GFP_DMA32 not used)? Maybe something
> > for another day.
>
> swiotlb_init allocates low memory from meblock, which is roughly
> equivalent to GFP_DMA allocations, so we'll be fine.
>
> > > @@ -143,10 +141,15 @@ static int __init xen_mm_init(void)
> > > if (!xen_swiotlb_detect())
> > > return 0;
> > >
> > > - rc = xen_swiotlb_init();
> > > /* we can work with the default swiotlb */
> > > - if (rc < 0 && rc != -EEXIST)
> > > - return rc;
> > > + if (!io_tlb_default_mem.nslabs) {
> > > + if (!xen_initial_domain())
> > > + return -EINVAL;
> >
> > I don't think we need this xen_initial_domain() check. It is all
> > already sorted out by the xen_swiotlb_detect() check above.
>
> Is it?
>
> static inline int xen_swiotlb_detect(void)
> {
> if (!xen_domain())
> return 0;
> if (xen_feature(XENFEAT_direct_mapped))
> return 1;
> /* legacy case */
> if (!xen_feature(XENFEAT_not_direct_mapped) && xen_initial_domain())
> return 1;
> return 0;
> }
It used to be that we had a
if (!xen_initial_domain())
return -EINVAL;
check in the initialization of swiotlb-xen on ARM. Then we replaced it
with the more sophisticated xen_swiotlb_detect().
The reason is that swiotlb-xen on ARM relies on Dom0 being 1:1 mapped
(guest physical addresses == physical addresses). Recent changes in Xen
allowed also DomUs to be 1:1 mapped. Changes still under discussion will
allow Dom0 not to be 1:1 mapped.
So, before all the Xen-side changes, knowing what was going to happen, I
introduced a clearer interface: XENFEAT_direct_mapped and
XENFEAT_not_direct_mapped tell us whether the guest (Linux) is 1:1
mapped or not. If it is 1:1 mapped then Linux can take advantage of
swiotlb-xen. Now xen_swiotlb_detect() returns true if Linux is 1:1
mapped.
Then of course there is the legacy case. That's taken care of by:
if (!xen_feature(XENFEAT_not_direct_mapped) && xen_initial_domain())
return 1;
The intention is to say that if
XENFEAT_direct_mapped/XENFEAT_not_direct_mapped are not present, then
use xen_initial_domain() like we did before.
So if xen_swiotlb_detect() returns true we know that Linux is either
dom0 (xen_initial_domain() == true) or we have very good reasons to
think we should initialize swiotlb-xen anyway
(xen_feature(XENFEAT_direct_mapped) == true).
> I think I'd keep it as-is for now, as my planned next step would be to
> fold xen-swiotlb into swiotlb entirely.
Thinking more about it we actually need to drop the xen_initial_domain()
check otherwise some cases won't be functional (Dom0 not 1:1 mapped, or
DomU 1:1 mapped).
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 11/12] swiotlb: merge swiotlb-xen initialization into swiotlb
On Tue, 1 Mar 2022, Christoph Hellwig wrote:
> Allow to pass a remap argument to the swiotlb initialization functions
> to handle the Xen/x86 remap case. ARM/ARM64 never did any remapping
> from xen_swiotlb_fixup, so we don't even need that quirk.
>
> Signed-off-by: Christoph Hellwig
> ---
> arch/arm/xen/mm.c | 23 +++---
> arch/x86/include/asm/xen/page.h | 5 --
> arch/x86/kernel/pci-dma.c | 19 +++--
> arch/x86/pci/sta2x11-fixup.c| 2 +-
> drivers/xen/swiotlb-xen.c | 128 +---
> include/linux/swiotlb.h | 7 +-
> include/xen/arm/page.h | 1 -
> include/xen/swiotlb-xen.h | 8 +-
> kernel/dma/swiotlb.c| 120 +++---
> 9 files changed, 96 insertions(+), 217 deletions(-)
>
> diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
> index a7e54a087b802..58b40f87617d3 100644
> --- a/arch/arm/xen/mm.c
> +++ b/arch/arm/xen/mm.c
> @@ -23,22 +23,20 @@
> #include
> #include
>
> -unsigned long xen_get_swiotlb_free_pages(unsigned int order)
> +static gfp_t xen_swiotlb_gfp(void)
> {
> phys_addr_t base;
> - gfp_t flags = __GFP_NOWARN|__GFP_KSWAPD_RECLAIM;
> u64 i;
>
> for_each_mem_range(i, , NULL) {
> if (base < (phys_addr_t)0x) {
> if (IS_ENABLED(CONFIG_ZONE_DMA32))
> - flags |= __GFP_DMA32;
> - else
> - flags |= __GFP_DMA;
> - break;
> + return __GFP_DMA32;
> + return __GFP_DMA;
> }
> }
> - return __get_free_pages(flags, order);
> +
> + return GFP_KERNEL;
> }
Unrelated to this specific patch series: now that I think about it, if
io_tlb_default_mem.nslabs is already allocated by the time xen_mm_init
is called, wouldn't we potentially have an issue with the GFP flags used
for the earlier allocation (e.g. GFP_DMA32 not used)? Maybe something
for another day.
> static bool hypercall_cflush = false;
> @@ -143,10 +141,15 @@ static int __init xen_mm_init(void)
> if (!xen_swiotlb_detect())
> return 0;
>
> - rc = xen_swiotlb_init();
> /* we can work with the default swiotlb */
> - if (rc < 0 && rc != -EEXIST)
> - return rc;
> + if (!io_tlb_default_mem.nslabs) {
> + if (!xen_initial_domain())
> + return -EINVAL;
I don't think we need this xen_initial_domain() check. It is all
already sorted out by the xen_swiotlb_detect() check above.
Aside from that the rest looks OK. Also, you can add my:
Tested-by: Stefano Stabellini
> + rc = swiotlb_init_late(swiotlb_size_or_default(),
> +xen_swiotlb_gfp(), NULL);
> + if (rc < 0)
> + return rc;
> + }
>
> cflush.op = 0;
> cflush.a.dev_bus_addr = 0;
> diff --git a/arch/x86/include/asm/xen/page.h b/arch/x86/include/asm/xen/page.h
> index e989bc2269f54..1fc67df500145 100644
> --- a/arch/x86/include/asm/xen/page.h
> +++ b/arch/x86/include/asm/xen/page.h
> @@ -357,9 +357,4 @@ static inline bool xen_arch_need_swiotlb(struct device
> *dev,
> return false;
> }
>
> -static inline unsigned long xen_get_swiotlb_free_pages(unsigned int order)
> -{
> - return __get_free_pages(__GFP_NOWARN, order);
> -}
> -
> #endif /* _ASM_X86_XEN_PAGE_H */
> diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c
> index e0def4b1c3181..2f2c468acb955 100644
> --- a/arch/x86/kernel/pci-dma.c
> +++ b/arch/x86/kernel/pci-dma.c
> @@ -71,15 +71,12 @@ static inline void __init pci_swiotlb_detect(void)
> #endif /* CONFIG_SWIOTLB */
>
> #ifdef CONFIG_SWIOTLB_XEN
> -static bool xen_swiotlb;
> -
> static void __init pci_xen_swiotlb_init(void)
> {
> if (!xen_initial_domain() && !x86_swiotlb_enable)
> return;
> x86_swiotlb_enable = false;
> - xen_swiotlb = true;
> - xen_swiotlb_init_early();
> + swiotlb_init_remap(true, x86_swiotlb_flags, xen_swiotlb_fixup);
> dma_ops = _swiotlb_dma_ops;
> if (IS_ENABLED(CONFIG_PCI))
> pci_request_acs();
> @@ -87,14 +84,16 @@ static void __init pci_xen_swiotlb_init(void)
>
> int pci_xen_swiotlb_init_late(void)
> {
> - int rc;
> -
> - if (xen_swiotlb)
> + if (dma_ops == _swiotlb_dma_ops)
> return 0;
>
> - rc = xen_swiotlb_init();
> - if (rc)
> - return rc;
> + /* we can work with the default swiotlb */
> + if (
Re: [PATCH v2 1/4] swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests
On Fri, 17 Sep 2021, Jan Beulich wrote:
> While the hypervisor hasn't been enforcing this, we would still better
> avoid issuing requests with GFNs not aligned to the requested order.
> Instead of altering the value also in the call to panic(), drop it
> there for being static and hence easy to determine without being part
> of the panic message.
>
> Signed-off-by: Jan Beulich
Reviewed-by: Stefano Stabellini
> ---
> I question how useful it is to wrap "bytes" in PAGE_ALIGN(), when it is
> a multiple of a segment's size anyway (or at least was supposed to be,
> prior to "swiotlb-xen: maintain slab count properly"). But that's
> perhaps yet another separate patch.
> ---
> v2: Drop logging of alignment. Wrap lines.
>
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -230,10 +230,11 @@ retry:
> /*
>* Get IO TLB memory from any location.
>*/
> - start = memblock_alloc(PAGE_ALIGN(bytes), PAGE_SIZE);
> + start = memblock_alloc(PAGE_ALIGN(bytes),
> +IO_TLB_SEGSIZE << IO_TLB_SHIFT);
> if (!start)
> - panic("%s: Failed to allocate %lu bytes align=0x%lx\n",
> - __func__, PAGE_ALIGN(bytes), PAGE_SIZE);
> + panic("%s: Failed to allocate %lu bytes\n",
> + __func__, PAGE_ALIGN(bytes));
>
> /*
>* And replace that memory with pages under 4GB.
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] swiotlb: set IO TLB segment size via cmdline
On Tue, 14 Sep 2021, Christoph Hellwig wrote: > On Tue, Sep 14, 2021 at 05:29:07PM +0200, Jan Beulich wrote: > > I'm not convinced the swiotlb use describe there falls under "intended > > use" - mapping a 1280x720 framebuffer in a single chunk? (As an aside, > > the bottom of this page is also confusing, as following "Then we can > > confirm the modified swiotlb size in the boot log:" there is a log > > fragment showing the same original size of 64Mb. > > It doesn't. We also do not add hacks to the kernel for whacky out > of tree modules. Also, Option 1 listed in the webpage seems to be a lot better. Any reason you can't do that? Because that option both solves the problem and increases performance. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [GIT PULL] dma-mapping fix for Linux 5.14
On Mon, 26 Jul 2021, Boris Ostrovsky wrote:
> On 7/25/21 12:50 PM, Linus Torvalds wrote:
> > On Sat, Jul 24, 2021 at 11:03 PM Christoph Hellwig
> > wrote:
> >
> >> - handle vmalloc addresses in dma_common_{mmap,get_sgtable}
> >> (Roman Skakun)
> > I've pulled this, but my reaction is that we've tried to avoid this in
> > the past. Why is Xen using vmalloc'ed addresses and passing those in
> > to the dma mapping routines?
> >
> > It *smells* to me like a Xen-swiotlb bug, and it would have been
> > better to try to fix it there. Was that just too painful?
>
>
> Stefano will probably know better but this appears to have something to do
> with how Pi (and possibly more ARM systems?) manage DMA memory:
> https://lore.kernel.org/xen-devel/CADz_WD5Ln7Pe1WAFp73d2Mz9wxspzTE3WgAJusp5S8LX4=8...@mail.gmail.com/.
The original issue was found on the Raspberry Pi 4, and the fix was in
swiotlb-xen.c, commit 8b1e868f6. More recently, Roman realized that
dma_common_mmap might also end up calling virt_to_page on a vmalloc
address. This is the fix for that.
Why is Xen using vmalloc'ed addresses with dma routines at all?
Xen is actually just calling the regular dma_direct_alloc to allocate
pages (xen_swiotlb_alloc_coherent -> xen_alloc_coherent_pages ->
dma_direct_alloc). dma_direct_alloc is the generic implementation. Back
when the original issue was found, dma_direct_alloc returned a vmalloc
address on RPi4.
The original analysis was "xen_alloc_coherent_pages() eventually calls
arch_dma_alloc() in remap.c which successfully allocates pages from
atomic pool." See https://marc.info/?l=xen-devel=158878173207775.
I don't know on which platform Roman Skakun (CC'ed) found the problem.
But if we look at arch/arm/mm/dma-mapping.c:__dma_alloc, one of the
possible options is the "remap_allocator", which calls
__alloc_remap_buffer, which calls dma_common_contiguous_remap, which
calls vmap.
So unfortunately it seems that on certain arch/platforms
dma_alloc_coherent can return a vmap'ed address. So I would imagine this
issue could also happen on native (without Xen), at least in theory.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] dma-mapping: use vmalloc_to_page for vmalloc addresses
On Fri, 16 Jul 2021, Roman Skakun wrote:
> > Technically this looks good. But given that exposing a helper
> > that does either vmalloc_to_page or virt_to_page is one of the
> > never ending MM discussions I don't want to get into that discussion
> > and just keep it local in the DMA code.
> >
> > Are you fine with me applying this version?
>
> Looks good to me, thanks!
> But, Stefano asked me about using created helper in the
> xen_swiotlb_free_coherent()
> and I created a patch according to this mention.
>
> We can merge this patch and create a new one for
> xen_swiotlb_free_coherent() later.
Yeah, no worries, I didn't know that exposing dma_common_vaddr_to_page
was problematic.
This patch is fine by me.
> пт, 16 июл. 2021 г. в 12:35, Christoph Hellwig :
> >
> > Technically this looks good. But given that exposing a helper
> > that does either vmalloc_to_page or virt_to_page is one of the
> > never ending MM discussions I don't want to get into that discussion
> > and just keep it local in the DMA code.
> >
> > Are you fine with me applying this version?
> >
> > ---
> > From 40ac971eab89330d6153e7721e88acd2d98833f9 Mon Sep 17 00:00:00 2001
> > From: Roman Skakun
> > Date: Fri, 16 Jul 2021 11:39:34 +0300
> > Subject: dma-mapping: handle vmalloc addresses in
> > dma_common_{mmap,get_sgtable}
> >
> > xen-swiotlb can use vmalloc backed addresses for dma coherent allocations
> > and uses the common helpers. Properly handle them to unbreak Xen on
> > ARM platforms.
> >
> > Fixes: 1b65c4e5a9af ("swiotlb-xen: use xen_alloc/free_coherent_pages")
> > Signed-off-by: Roman Skakun
> > Reviewed-by: Andrii Anisov
> > [hch: split the patch, renamed the helpers]
> > Signed-off-by: Christoph Hellwig
> > ---
> > kernel/dma/ops_helpers.c | 12 ++--
> > 1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/dma/ops_helpers.c b/kernel/dma/ops_helpers.c
> > index 910ae69cae77..af4a6ef48ce0 100644
> > --- a/kernel/dma/ops_helpers.c
> > +++ b/kernel/dma/ops_helpers.c
> > @@ -5,6 +5,13 @@
> > */
> > #include
> >
> > +static struct page *dma_common_vaddr_to_page(void *cpu_addr)
> > +{
> > + if (is_vmalloc_addr(cpu_addr))
> > + return vmalloc_to_page(cpu_addr);
> > + return virt_to_page(cpu_addr);
> > +}
> > +
> > /*
> > * Create scatter-list for the already allocated DMA buffer.
> > */
> > @@ -12,7 +19,7 @@ int dma_common_get_sgtable(struct device *dev, struct
> > sg_table *sgt,
> > void *cpu_addr, dma_addr_t dma_addr, size_t size,
> > unsigned long attrs)
> > {
> > - struct page *page = virt_to_page(cpu_addr);
> > + struct page *page = dma_common_vaddr_to_page(cpu_addr);
> > int ret;
> >
> > ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
> > @@ -32,6 +39,7 @@ int dma_common_mmap(struct device *dev, struct
> > vm_area_struct *vma,
> > unsigned long user_count = vma_pages(vma);
> > unsigned long count = PAGE_ALIGN(size) >> PAGE_SHIFT;
> > unsigned long off = vma->vm_pgoff;
> > + struct page *page = dma_common_vaddr_to_page(cpu_addr);
> > int ret = -ENXIO;
> >
> > vma->vm_page_prot = dma_pgprot(dev, vma->vm_page_prot, attrs);
> > @@ -43,7 +51,7 @@ int dma_common_mmap(struct device *dev, struct
> > vm_area_struct *vma,
> > return -ENXIO;
> >
> > return remap_pfn_range(vma, vma->vm_start,
> > - page_to_pfn(virt_to_page(cpu_addr)) + vma->vm_pgoff,
> > + page_to_pfn(page) + vma->vm_pgoff,
> > user_count << PAGE_SHIFT, vma->vm_page_prot);
> > #else
> > return -ENXIO;
> > --
> > 2.30.2
> >
>
>
> --
> Best Regards, Roman.
> ___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] dma-mapping: use vmalloc_to_page for vmalloc addresses
On Tue, 22 Jun 2021, Roman Skakun wrote:
> This commit is dedicated to fix incorrect conversion from
> cpu_addr to page address in cases when we get virtual
> address which allocated in the vmalloc range.
> As the result, virt_to_page() cannot convert this address
> properly and return incorrect page address.
>
> Need to detect such cases and obtains the page address using
> vmalloc_to_page() instead.
>
> Signed-off-by: Roman Skakun
> Reviewed-by: Andrii Anisov
> ---
> Hey!
> Thanks for suggestions, Christoph!
> I updated the patch according to your advice.
> But, I'm so surprised because nobody catches this problem
> in the common code before. It looks a bit strange as for me.
>
>
> kernel/dma/ops_helpers.c | 12 ++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/dma/ops_helpers.c b/kernel/dma/ops_helpers.c
> index 910ae69cae77..782728d8a393 100644
> --- a/kernel/dma/ops_helpers.c
> +++ b/kernel/dma/ops_helpers.c
> @@ -5,6 +5,14 @@
> */
> #include
>
> +static struct page *cpu_addr_to_page(void *cpu_addr)
> +{
> + if (is_vmalloc_addr(cpu_addr))
> + return vmalloc_to_page(cpu_addr);
> + else
> + return virt_to_page(cpu_addr);
> +}
We have the same thing in xen_swiotlb_free_coherent. Can we find a way
to call cpu_addr_to_page() from xen_swiotlb_free_coherent? Maybe we can
make cpu_addr_to_page non-static? No problem if it is too much trouble.
> /*
> * Create scatter-list for the already allocated DMA buffer.
> */
> @@ -12,7 +20,7 @@ int dma_common_get_sgtable(struct device *dev, struct
> sg_table *sgt,
>void *cpu_addr, dma_addr_t dma_addr, size_t size,
>unsigned long attrs)
> {
> - struct page *page = virt_to_page(cpu_addr);
> + struct page *page = cpu_addr_to_page(cpu_addr);
> int ret;
>
> ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
> @@ -43,7 +51,7 @@ int dma_common_mmap(struct device *dev, struct
> vm_area_struct *vma,
> return -ENXIO;
>
> return remap_pfn_range(vma, vma->vm_start,
> - page_to_pfn(virt_to_page(cpu_addr)) + vma->vm_pgoff,
> + page_to_pfn(cpu_addr_to_page(cpu_addr)) + vma->vm_pgoff,
> user_count << PAGE_SHIFT, vma->vm_page_prot);
> #else
> return -ENXIO;
> --
> 2.25.1
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v14 01/12] swiotlb: Refactor swiotlb init functions
On Sat, 19 Jun 2021, Claire Chang wrote:
> Add a new function, swiotlb_init_io_tlb_mem, for the io_tlb_mem struct
> initialization to make the code reusable.
>
> Signed-off-by: Claire Chang
> Reviewed-by: Christoph Hellwig
> Tested-by: Stefano Stabellini
> Tested-by: Will Deacon
Acked-by: Stefano Stabellini
> ---
> kernel/dma/swiotlb.c | 50 ++--
> 1 file changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index 52e2ac526757..1f9b2b9e7490 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -168,9 +168,28 @@ void __init swiotlb_update_mem_attributes(void)
> memset(vaddr, 0, bytes);
> }
>
> -int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int
> verbose)
> +static void swiotlb_init_io_tlb_mem(struct io_tlb_mem *mem, phys_addr_t
> start,
> + unsigned long nslabs, bool late_alloc)
> {
> + void *vaddr = phys_to_virt(start);
> unsigned long bytes = nslabs << IO_TLB_SHIFT, i;
> +
> + mem->nslabs = nslabs;
> + mem->start = start;
> + mem->end = mem->start + bytes;
> + mem->index = 0;
> + mem->late_alloc = late_alloc;
> + spin_lock_init(>lock);
> + for (i = 0; i < mem->nslabs; i++) {
> + mem->slots[i].list = IO_TLB_SEGSIZE - io_tlb_offset(i);
> + mem->slots[i].orig_addr = INVALID_PHYS_ADDR;
> + mem->slots[i].alloc_size = 0;
> + }
> + memset(vaddr, 0, bytes);
> +}
> +
> +int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int
> verbose)
> +{
> struct io_tlb_mem *mem;
> size_t alloc_size;
>
> @@ -186,16 +205,8 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned
> long nslabs, int verbose)
> if (!mem)
> panic("%s: Failed to allocate %zu bytes align=0x%lx\n",
> __func__, alloc_size, PAGE_SIZE);
> - mem->nslabs = nslabs;
> - mem->start = __pa(tlb);
> - mem->end = mem->start + bytes;
> - mem->index = 0;
> - spin_lock_init(>lock);
> - for (i = 0; i < mem->nslabs; i++) {
> - mem->slots[i].list = IO_TLB_SEGSIZE - io_tlb_offset(i);
> - mem->slots[i].orig_addr = INVALID_PHYS_ADDR;
> - mem->slots[i].alloc_size = 0;
> - }
> +
> + swiotlb_init_io_tlb_mem(mem, __pa(tlb), nslabs, false);
>
> io_tlb_default_mem = mem;
> if (verbose)
> @@ -282,8 +293,8 @@ swiotlb_late_init_with_default_size(size_t default_size)
> int
> swiotlb_late_init_with_tbl(char *tlb, unsigned long nslabs)
> {
> - unsigned long bytes = nslabs << IO_TLB_SHIFT, i;
> struct io_tlb_mem *mem;
> + unsigned long bytes = nslabs << IO_TLB_SHIFT;
>
> if (swiotlb_force == SWIOTLB_NO_FORCE)
> return 0;
> @@ -297,20 +308,9 @@ swiotlb_late_init_with_tbl(char *tlb, unsigned long
> nslabs)
> if (!mem)
> return -ENOMEM;
>
> - mem->nslabs = nslabs;
> - mem->start = virt_to_phys(tlb);
> - mem->end = mem->start + bytes;
> - mem->index = 0;
> - mem->late_alloc = 1;
> - spin_lock_init(>lock);
> - for (i = 0; i < mem->nslabs; i++) {
> - mem->slots[i].list = IO_TLB_SEGSIZE - io_tlb_offset(i);
> - mem->slots[i].orig_addr = INVALID_PHYS_ADDR;
> - mem->slots[i].alloc_size = 0;
> - }
> -
> + memset(mem, 0, sizeof(*mem));
> set_memory_decrypted((unsigned long)tlb, bytes >> PAGE_SHIFT);
> - memset(tlb, 0, bytes);
> + swiotlb_init_io_tlb_mem(mem, virt_to_phys(tlb), nslabs, true);
>
> io_tlb_default_mem = mem;
> swiotlb_print_info();
> --
> 2.32.0.288.g62a8d224e6-goog
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v13 01/12] swiotlb: Refactor swiotlb init functions
On Fri, 18 Jun 2021, Christoph Hellwig wrote: > On Fri, Jun 18, 2021 at 09:09:17AM -0500, Tom Lendacky wrote: > > > swiotlb_init_with_tbl uses memblock_alloc to allocate the io_tlb_mem > > > and memblock_alloc[1] will do memset in memblock_alloc_try_nid[2], so > > > swiotlb_init_with_tbl is also good. > > > I'm happy to add the memset in swiotlb_init_io_tlb_mem if you think > > > it's clearer and safer. > > > > On x86, if the memset is done before set_memory_decrypted() and memory > > encryption is active, then the memory will look like ciphertext afterwards > > and not be zeroes. If zeroed memory is required, then a memset must be > > done after the set_memory_decrypted() calls. > > Which should be fine - we don't care that the memory is cleared to 0, > just that it doesn't leak other data. Maybe a comment would be useful, > though, Just as a clarification: I was referring to the zeroing of "mem" in swiotlb_late_init_with_tbl and swiotlb_init_with_tbl. While it looks like Tom and Christoph are talking about the zeroing of "tlb". The zeroing of "mem" is required as some fields of struct io_tlb_mem need to be initialized to zero. While the zeroing of "tlb" is not required except from the point of view of not leaking sensitive data as Christoph mentioned. Either way, Claire's v14 patch 01/12 looks fine to me. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v13 09/12] swiotlb: Add restricted DMA alloc/free support
On Thu, 17 Jun 2021, Claire Chang wrote:
> Add the functions, swiotlb_{alloc,free} and is_swiotlb_for_alloc to
> support the memory allocation from restricted DMA pool.
>
> The restricted DMA pool is preferred if available.
>
> Note that since coherent allocation needs remapping, one must set up
> another device coherent pool by shared-dma-pool and use
> dma_alloc_from_dev_coherent instead for atomic coherent allocation.
>
> Signed-off-by: Claire Chang
> Reviewed-by: Christoph Hellwig
> Tested-by: Stefano Stabellini
> Tested-by: Will Deacon
Acked-by: Stefano Stabellini
> ---
> include/linux/swiotlb.h | 26 ++
> kernel/dma/direct.c | 49 +++--
> kernel/dma/swiotlb.c| 38 ++--
> 3 files changed, 99 insertions(+), 14 deletions(-)
>
> diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
> index 8d8855c77d9a..a73fad460162 100644
> --- a/include/linux/swiotlb.h
> +++ b/include/linux/swiotlb.h
> @@ -85,6 +85,7 @@ extern enum swiotlb_force swiotlb_force;
> * @debugfs: The dentry to debugfs.
> * @late_alloc: %true if allocated using the page allocator
> * @force_bounce: %true if swiotlb bouncing is forced
> + * @for_alloc: %true if the pool is used for memory allocation
> */
> struct io_tlb_mem {
> phys_addr_t start;
> @@ -96,6 +97,7 @@ struct io_tlb_mem {
> struct dentry *debugfs;
> bool late_alloc;
> bool force_bounce;
> + bool for_alloc;
> struct io_tlb_slot {
> phys_addr_t orig_addr;
> size_t alloc_size;
> @@ -156,4 +158,28 @@ static inline void swiotlb_adjust_size(unsigned long
> size)
> extern void swiotlb_print_info(void);
> extern void swiotlb_set_max_segment(unsigned int);
>
> +#ifdef CONFIG_DMA_RESTRICTED_POOL
> +struct page *swiotlb_alloc(struct device *dev, size_t size);
> +bool swiotlb_free(struct device *dev, struct page *page, size_t size);
> +
> +static inline bool is_swiotlb_for_alloc(struct device *dev)
> +{
> + return dev->dma_io_tlb_mem->for_alloc;
> +}
> +#else
> +static inline struct page *swiotlb_alloc(struct device *dev, size_t size)
> +{
> + return NULL;
> +}
> +static inline bool swiotlb_free(struct device *dev, struct page *page,
> + size_t size)
> +{
> + return false;
> +}
> +static inline bool is_swiotlb_for_alloc(struct device *dev)
> +{
> + return false;
> +}
> +#endif /* CONFIG_DMA_RESTRICTED_POOL */
> +
> #endif /* __LINUX_SWIOTLB_H */
> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
> index a92465b4eb12..2de33e5d302b 100644
> --- a/kernel/dma/direct.c
> +++ b/kernel/dma/direct.c
> @@ -75,6 +75,15 @@ static bool dma_coherent_ok(struct device *dev,
> phys_addr_t phys, size_t size)
> min_not_zero(dev->coherent_dma_mask, dev->bus_dma_limit);
> }
>
> +static void __dma_direct_free_pages(struct device *dev, struct page *page,
> + size_t size)
> +{
> + if (IS_ENABLED(CONFIG_DMA_RESTRICTED_POOL) &&
> + swiotlb_free(dev, page, size))
> + return;
> + dma_free_contiguous(dev, page, size);
> +}
> +
> static struct page *__dma_direct_alloc_pages(struct device *dev, size_t size,
> gfp_t gfp)
> {
> @@ -86,6 +95,16 @@ static struct page *__dma_direct_alloc_pages(struct device
> *dev, size_t size,
>
> gfp |= dma_direct_optimal_gfp_mask(dev, dev->coherent_dma_mask,
> _limit);
> + if (IS_ENABLED(CONFIG_DMA_RESTRICTED_POOL) &&
> + is_swiotlb_for_alloc(dev)) {
> + page = swiotlb_alloc(dev, size);
> + if (page && !dma_coherent_ok(dev, page_to_phys(page), size)) {
> + __dma_direct_free_pages(dev, page, size);
> + return NULL;
> + }
> + return page;
> + }
> +
> page = dma_alloc_contiguous(dev, size, gfp);
> if (page && !dma_coherent_ok(dev, page_to_phys(page), size)) {
> dma_free_contiguous(dev, page, size);
> @@ -142,7 +161,7 @@ void *dma_direct_alloc(struct device *dev, size_t size,
> gfp |= __GFP_NOWARN;
>
> if ((attrs & DMA_ATTR_NO_KERNEL_MAPPING) &&
> - !force_dma_unencrypted(dev)) {
> + !force_dma_unencrypted(dev) && !is_swiotlb_for_alloc(dev)) {
> page = __dma_direct_alloc_pages(dev, size, gfp & ~__GFP_ZERO);
> if (!page)
> return NULL;
> @@ -155,18
Re: [PATCH v13 06/12] swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
On Thu, 17 Jun 2021, Claire Chang wrote:
> Propagate the swiotlb_force into io_tlb_default_mem->force_bounce and
> use it to determine whether to bounce the data or not. This will be
> useful later to allow for different pools.
>
> Signed-off-by: Claire Chang
> Reviewed-by: Christoph Hellwig
> Tested-by: Stefano Stabellini
> Tested-by: Will Deacon
Acked-by: Stefano Stabellini
> ---
> drivers/xen/swiotlb-xen.c | 2 +-
> include/linux/swiotlb.h | 11 +++
> kernel/dma/direct.c | 2 +-
> kernel/dma/direct.h | 2 +-
> kernel/dma/swiotlb.c | 4
> 5 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index 0c6ed09f8513..4730a146fa35 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -369,7 +369,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device
> *dev, struct page *page,
> if (dma_capable(dev, dev_addr, size, true) &&
> !range_straddles_page_boundary(phys, size) &&
> !xen_arch_need_swiotlb(dev, phys, dev_addr) &&
> - swiotlb_force != SWIOTLB_FORCE)
> + !is_swiotlb_force_bounce(dev))
> goto done;
>
> /*
> diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
> index dd1c30a83058..8d8855c77d9a 100644
> --- a/include/linux/swiotlb.h
> +++ b/include/linux/swiotlb.h
> @@ -84,6 +84,7 @@ extern enum swiotlb_force swiotlb_force;
> * unmap calls.
> * @debugfs: The dentry to debugfs.
> * @late_alloc: %true if allocated using the page allocator
> + * @force_bounce: %true if swiotlb bouncing is forced
> */
> struct io_tlb_mem {
> phys_addr_t start;
> @@ -94,6 +95,7 @@ struct io_tlb_mem {
> spinlock_t lock;
> struct dentry *debugfs;
> bool late_alloc;
> + bool force_bounce;
> struct io_tlb_slot {
> phys_addr_t orig_addr;
> size_t alloc_size;
> @@ -109,6 +111,11 @@ static inline bool is_swiotlb_buffer(struct device *dev,
> phys_addr_t paddr)
> return mem && paddr >= mem->start && paddr < mem->end;
> }
>
> +static inline bool is_swiotlb_force_bounce(struct device *dev)
> +{
> + return dev->dma_io_tlb_mem->force_bounce;
> +}
> +
> void __init swiotlb_exit(void);
> unsigned int swiotlb_max_segment(void);
> size_t swiotlb_max_mapping_size(struct device *dev);
> @@ -120,6 +127,10 @@ static inline bool is_swiotlb_buffer(struct device *dev,
> phys_addr_t paddr)
> {
> return false;
> }
> +static inline bool is_swiotlb_force_bounce(struct device *dev)
> +{
> + return false;
> +}
> static inline void swiotlb_exit(void)
> {
> }
> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
> index 7a88c34d0867..a92465b4eb12 100644
> --- a/kernel/dma/direct.c
> +++ b/kernel/dma/direct.c
> @@ -496,7 +496,7 @@ size_t dma_direct_max_mapping_size(struct device *dev)
> {
> /* If SWIOTLB is active, use its maximum mapping size */
> if (is_swiotlb_active(dev) &&
> - (dma_addressing_limited(dev) || swiotlb_force == SWIOTLB_FORCE))
> + (dma_addressing_limited(dev) || is_swiotlb_force_bounce(dev)))
> return swiotlb_max_mapping_size(dev);
> return SIZE_MAX;
> }
> diff --git a/kernel/dma/direct.h b/kernel/dma/direct.h
> index 13e9e7158d94..4632b0f4f72e 100644
> --- a/kernel/dma/direct.h
> +++ b/kernel/dma/direct.h
> @@ -87,7 +87,7 @@ static inline dma_addr_t dma_direct_map_page(struct device
> *dev,
> phys_addr_t phys = page_to_phys(page) + offset;
> dma_addr_t dma_addr = phys_to_dma(dev, phys);
>
> - if (unlikely(swiotlb_force == SWIOTLB_FORCE))
> + if (is_swiotlb_force_bounce(dev))
> return swiotlb_map(dev, phys, size, dir, attrs);
>
> if (unlikely(!dma_capable(dev, dma_addr, size, true))) {
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index 409694d7a8ad..13891d5de8c9 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -179,6 +179,10 @@ static void swiotlb_init_io_tlb_mem(struct io_tlb_mem
> *mem, phys_addr_t start,
> mem->end = mem->start + bytes;
> mem->index = 0;
> mem->late_alloc = late_alloc;
> +
> + if (swiotlb_force == SWIOTLB_FORCE)
> + mem->force_bounce = true;
> +
> spin_lock_init(>lock);
> for (i = 0; i < mem->nslabs; i++) {
> mem->slots[i].list = IO_TLB_SEGSIZE - io_tlb_offset(i);
> --
> 2.32.0.288.g62a8d224e6-goog
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v13 05/12] swiotlb: Update is_swiotlb_active to add a struct device argument
On Thu, 17 Jun 2021, Claire Chang wrote:
> Update is_swiotlb_active to add a struct device argument. This will be
> useful later to allow for different pools.
>
> Signed-off-by: Claire Chang
> Reviewed-by: Christoph Hellwig
> Tested-by: Stefano Stabellini
> Tested-by: Will Deacon
Acked-by: Stefano Stabellini
> ---
> drivers/gpu/drm/i915/gem/i915_gem_internal.c | 2 +-
> drivers/gpu/drm/nouveau/nouveau_ttm.c| 2 +-
> drivers/pci/xen-pcifront.c | 2 +-
> include/linux/swiotlb.h | 4 ++--
> kernel/dma/direct.c | 2 +-
> kernel/dma/swiotlb.c | 4 ++--
> 6 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_internal.c
> b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
> index a9d65fc8aa0e..4b7afa0fc85d 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_internal.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
> @@ -42,7 +42,7 @@ static int i915_gem_object_get_pages_internal(struct
> drm_i915_gem_object *obj)
>
> max_order = MAX_ORDER;
> #ifdef CONFIG_SWIOTLB
> - if (is_swiotlb_active()) {
> + if (is_swiotlb_active(obj->base.dev->dev)) {
> unsigned int max_segment;
>
> max_segment = swiotlb_max_segment();
> diff --git a/drivers/gpu/drm/nouveau/nouveau_ttm.c
> b/drivers/gpu/drm/nouveau/nouveau_ttm.c
> index 9662522aa066..be15bfd9e0ee 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_ttm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_ttm.c
> @@ -321,7 +321,7 @@ nouveau_ttm_init(struct nouveau_drm *drm)
> }
>
> #if IS_ENABLED(CONFIG_SWIOTLB) && IS_ENABLED(CONFIG_X86)
> - need_swiotlb = is_swiotlb_active();
> + need_swiotlb = is_swiotlb_active(dev->dev);
> #endif
>
> ret = ttm_bo_device_init(>ttm.bdev, _bo_driver,
> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
> index b7a8f3a1921f..0d56985bfe81 100644
> --- a/drivers/pci/xen-pcifront.c
> +++ b/drivers/pci/xen-pcifront.c
> @@ -693,7 +693,7 @@ static int pcifront_connect_and_init_dma(struct
> pcifront_device *pdev)
>
> spin_unlock(_dev_lock);
>
> - if (!err && !is_swiotlb_active()) {
> + if (!err && !is_swiotlb_active(>xdev->dev)) {
> err = pci_xen_swiotlb_init_late();
> if (err)
> dev_err(>xdev->dev, "Could not setup SWIOTLB!\n");
> diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
> index d1f3d95881cd..dd1c30a83058 100644
> --- a/include/linux/swiotlb.h
> +++ b/include/linux/swiotlb.h
> @@ -112,7 +112,7 @@ static inline bool is_swiotlb_buffer(struct device *dev,
> phys_addr_t paddr)
> void __init swiotlb_exit(void);
> unsigned int swiotlb_max_segment(void);
> size_t swiotlb_max_mapping_size(struct device *dev);
> -bool is_swiotlb_active(void);
> +bool is_swiotlb_active(struct device *dev);
> void __init swiotlb_adjust_size(unsigned long size);
> #else
> #define swiotlb_force SWIOTLB_NO_FORCE
> @@ -132,7 +132,7 @@ static inline size_t swiotlb_max_mapping_size(struct
> device *dev)
> return SIZE_MAX;
> }
>
> -static inline bool is_swiotlb_active(void)
> +static inline bool is_swiotlb_active(struct device *dev)
> {
> return false;
> }
> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
> index 84c9feb5474a..7a88c34d0867 100644
> --- a/kernel/dma/direct.c
> +++ b/kernel/dma/direct.c
> @@ -495,7 +495,7 @@ int dma_direct_supported(struct device *dev, u64 mask)
> size_t dma_direct_max_mapping_size(struct device *dev)
> {
> /* If SWIOTLB is active, use its maximum mapping size */
> - if (is_swiotlb_active() &&
> + if (is_swiotlb_active(dev) &&
> (dma_addressing_limited(dev) || swiotlb_force == SWIOTLB_FORCE))
> return swiotlb_max_mapping_size(dev);
> return SIZE_MAX;
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index de79e9437030..409694d7a8ad 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -664,9 +664,9 @@ size_t swiotlb_max_mapping_size(struct device *dev)
> return ((size_t)IO_TLB_SIZE) * IO_TLB_SEGSIZE;
> }
>
> -bool is_swiotlb_active(void)
> +bool is_swiotlb_active(struct device *dev)
> {
> - return io_tlb_default_mem != NULL;
> + return dev->dma_io_tlb_mem != NULL;
> }
> EXPORT_SYMBOL_GPL(is_swiotlb_active);
>
> --
> 2.32.0.288.g62a8d224e6-goog
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v13 04/12] swiotlb: Update is_swiotlb_buffer to add a struct device argument
On Thu, 17 Jun 2021, Claire Chang wrote:
> Update is_swiotlb_buffer to add a struct device argument. This will be
> useful later to allow for different pools.
>
> Signed-off-by: Claire Chang
> Reviewed-by: Christoph Hellwig
> Tested-by: Stefano Stabellini
> Tested-by: Will Deacon
Acked-by: Stefano Stabellini
> ---
> drivers/iommu/dma-iommu.c | 12 ++--
> drivers/xen/swiotlb-xen.c | 2 +-
> include/linux/swiotlb.h | 7 ---
> kernel/dma/direct.c | 6 +++---
> kernel/dma/direct.h | 6 +++---
> 5 files changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 3087d9fa6065..10997ef541f8 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -507,7 +507,7 @@ static void __iommu_dma_unmap_swiotlb(struct device *dev,
> dma_addr_t dma_addr,
>
> __iommu_dma_unmap(dev, dma_addr, size);
>
> - if (unlikely(is_swiotlb_buffer(phys)))
> + if (unlikely(is_swiotlb_buffer(dev, phys)))
> swiotlb_tbl_unmap_single(dev, phys, size, dir, attrs);
> }
>
> @@ -578,7 +578,7 @@ static dma_addr_t __iommu_dma_map_swiotlb(struct device
> *dev, phys_addr_t phys,
> }
>
> iova = __iommu_dma_map(dev, phys, aligned_size, prot, dma_mask);
> - if (iova == DMA_MAPPING_ERROR && is_swiotlb_buffer(phys))
> + if (iova == DMA_MAPPING_ERROR && is_swiotlb_buffer(dev, phys))
> swiotlb_tbl_unmap_single(dev, phys, org_size, dir, attrs);
> return iova;
> }
> @@ -749,7 +749,7 @@ static void iommu_dma_sync_single_for_cpu(struct device
> *dev,
> if (!dev_is_dma_coherent(dev))
> arch_sync_dma_for_cpu(phys, size, dir);
>
> - if (is_swiotlb_buffer(phys))
> + if (is_swiotlb_buffer(dev, phys))
> swiotlb_sync_single_for_cpu(dev, phys, size, dir);
> }
>
> @@ -762,7 +762,7 @@ static void iommu_dma_sync_single_for_device(struct
> device *dev,
> return;
>
> phys = iommu_iova_to_phys(iommu_get_dma_domain(dev), dma_handle);
> - if (is_swiotlb_buffer(phys))
> + if (is_swiotlb_buffer(dev, phys))
> swiotlb_sync_single_for_device(dev, phys, size, dir);
>
> if (!dev_is_dma_coherent(dev))
> @@ -783,7 +783,7 @@ static void iommu_dma_sync_sg_for_cpu(struct device *dev,
> if (!dev_is_dma_coherent(dev))
> arch_sync_dma_for_cpu(sg_phys(sg), sg->length, dir);
>
> - if (is_swiotlb_buffer(sg_phys(sg)))
> + if (is_swiotlb_buffer(dev, sg_phys(sg)))
> swiotlb_sync_single_for_cpu(dev, sg_phys(sg),
> sg->length, dir);
> }
> @@ -800,7 +800,7 @@ static void iommu_dma_sync_sg_for_device(struct device
> *dev,
> return;
>
> for_each_sg(sgl, sg, nelems, i) {
> - if (is_swiotlb_buffer(sg_phys(sg)))
> + if (is_swiotlb_buffer(dev, sg_phys(sg)))
> swiotlb_sync_single_for_device(dev, sg_phys(sg),
> sg->length, dir);
>
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index 4c89afc0df62..0c6ed09f8513 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -100,7 +100,7 @@ static int is_xen_swiotlb_buffer(struct device *dev,
> dma_addr_t dma_addr)
>* in our domain. Therefore _only_ check address within our domain.
>*/
> if (pfn_valid(PFN_DOWN(paddr)))
> - return is_swiotlb_buffer(paddr);
> + return is_swiotlb_buffer(dev, paddr);
> return 0;
> }
>
> diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
> index 216854a5e513..d1f3d95881cd 100644
> --- a/include/linux/swiotlb.h
> +++ b/include/linux/swiotlb.h
> @@ -2,6 +2,7 @@
> #ifndef __LINUX_SWIOTLB_H
> #define __LINUX_SWIOTLB_H
>
> +#include
> #include
> #include
> #include
> @@ -101,9 +102,9 @@ struct io_tlb_mem {
> };
> extern struct io_tlb_mem *io_tlb_default_mem;
>
> -static inline bool is_swiotlb_buffer(phys_addr_t paddr)
> +static inline bool is_swiotlb_buffer(struct device *dev, phys_addr_t paddr)
> {
> - struct io_tlb_mem *mem = io_tlb_default_mem;
> + struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
>
> return mem && paddr >= mem->start && paddr < mem->end;
> }
> @@ -115,7 +116,7 @@ bool is_swiotlb_active(void);
> void __init swiotlb_adjust_size(unsigned long size);
> #else
> #define swiotlb_force SWIOTLB_NO_FORCE
> -static
Re: [PATCH v13 03/12] swiotlb: Set dev->dma_io_tlb_mem to the swiotlb pool used
On Thu, 17 Jun 2021, Claire Chang wrote:
> Always have the pointer to the swiotlb pool used in struct device. This
> could help simplify the code for other pools.
>
> Signed-off-by: Claire Chang
> Reviewed-by: Christoph Hellwig
> Tested-by: Stefano Stabellini
> Tested-by: Will Deacon
Acked-by: Stefano Stabellini
> ---
> drivers/base/core.c| 4
> include/linux/device.h | 4
> kernel/dma/swiotlb.c | 8
> 3 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index f29839382f81..cb3123e3954d 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -27,6 +27,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include /* for dma_default_coherent */
>
> @@ -2736,6 +2737,9 @@ void device_initialize(struct device *dev)
> defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
> dev->dma_coherent = dma_default_coherent;
> #endif
> +#ifdef CONFIG_SWIOTLB
> + dev->dma_io_tlb_mem = io_tlb_default_mem;
> +#endif
> }
> EXPORT_SYMBOL_GPL(device_initialize);
>
> diff --git a/include/linux/device.h b/include/linux/device.h
> index ba660731bd25..240d652a0696 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -416,6 +416,7 @@ struct dev_links_info {
> * @dma_pools: Dma pools (if dma'ble device).
> * @dma_mem: Internal for coherent mem override.
> * @cma_area:Contiguous memory area for dma allocations
> + * @dma_io_tlb_mem: Pointer to the swiotlb pool used. Not for driver use.
> * @archdata:For arch-specific additions.
> * @of_node: Associated device tree node.
> * @fwnode: Associated device node supplied by platform firmware.
> @@ -518,6 +519,9 @@ struct device {
> #ifdef CONFIG_DMA_CMA
> struct cma *cma_area; /* contiguous memory area for dma
> allocations */
> +#endif
> +#ifdef CONFIG_SWIOTLB
> + struct io_tlb_mem *dma_io_tlb_mem;
> #endif
> /* arch specific additions */
> struct dev_archdata archdata;
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index 2dba659a1e73..de79e9437030 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -340,7 +340,7 @@ void __init swiotlb_exit(void)
> static void swiotlb_bounce(struct device *dev, phys_addr_t tlb_addr, size_t
> size,
> enum dma_data_direction dir)
> {
> - struct io_tlb_mem *mem = io_tlb_default_mem;
> + struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
> int index = (tlb_addr - mem->start) >> IO_TLB_SHIFT;
> unsigned int offset = (tlb_addr - mem->start) & (IO_TLB_SIZE - 1);
> phys_addr_t orig_addr = mem->slots[index].orig_addr;
> @@ -431,7 +431,7 @@ static unsigned int wrap_index(struct io_tlb_mem *mem,
> unsigned int index)
> static int find_slots(struct device *dev, phys_addr_t orig_addr,
> size_t alloc_size)
> {
> - struct io_tlb_mem *mem = io_tlb_default_mem;
> + struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
> unsigned long boundary_mask = dma_get_seg_boundary(dev);
> dma_addr_t tbl_dma_addr =
> phys_to_dma_unencrypted(dev, mem->start) & boundary_mask;
> @@ -508,7 +508,7 @@ 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)
> {
> - struct io_tlb_mem *mem = io_tlb_default_mem;
> + struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
> unsigned int offset = swiotlb_align_offset(dev, orig_addr);
> unsigned int i;
> int index;
> @@ -559,7 +559,7 @@ void swiotlb_tbl_unmap_single(struct device *hwdev,
> phys_addr_t tlb_addr,
> size_t mapping_size, enum dma_data_direction dir,
> unsigned long attrs)
> {
> - struct io_tlb_mem *mem = io_tlb_default_mem;
> + struct io_tlb_mem *mem = hwdev->dma_io_tlb_mem;
> unsigned long flags;
> unsigned int offset = swiotlb_align_offset(hwdev, tlb_addr);
> int index = (tlb_addr - offset - mem->start) >> IO_TLB_SHIFT;
> --
> 2.32.0.288.g62a8d224e6-goog
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v13 01/12] swiotlb: Refactor swiotlb init functions
On Thu, 17 Jun 2021, Claire Chang wrote:
> Add a new function, swiotlb_init_io_tlb_mem, for the io_tlb_mem struct
> initialization to make the code reusable.
>
> Signed-off-by: Claire Chang
> Reviewed-by: Christoph Hellwig
> Tested-by: Stefano Stabellini
> Tested-by: Will Deacon
> ---
> kernel/dma/swiotlb.c | 50 ++--
> 1 file changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index 52e2ac526757..47bb2a766798 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -168,9 +168,28 @@ void __init swiotlb_update_mem_attributes(void)
> memset(vaddr, 0, bytes);
> }
>
> -int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int
> verbose)
> +static void swiotlb_init_io_tlb_mem(struct io_tlb_mem *mem, phys_addr_t
> start,
> + unsigned long nslabs, bool late_alloc)
> {
> + void *vaddr = phys_to_virt(start);
> unsigned long bytes = nslabs << IO_TLB_SHIFT, i;
> +
> + mem->nslabs = nslabs;
> + mem->start = start;
> + mem->end = mem->start + bytes;
> + mem->index = 0;
> + mem->late_alloc = late_alloc;
> + spin_lock_init(>lock);
> + for (i = 0; i < mem->nslabs; i++) {
> + mem->slots[i].list = IO_TLB_SEGSIZE - io_tlb_offset(i);
> + mem->slots[i].orig_addr = INVALID_PHYS_ADDR;
> + mem->slots[i].alloc_size = 0;
> + }
> + memset(vaddr, 0, bytes);
> +}
> +
> +int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int
> verbose)
> +{
> struct io_tlb_mem *mem;
> size_t alloc_size;
>
> @@ -186,16 +205,8 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned
> long nslabs, int verbose)
> if (!mem)
> panic("%s: Failed to allocate %zu bytes align=0x%lx\n",
> __func__, alloc_size, PAGE_SIZE);
> - mem->nslabs = nslabs;
> - mem->start = __pa(tlb);
> - mem->end = mem->start + bytes;
> - mem->index = 0;
> - spin_lock_init(>lock);
> - for (i = 0; i < mem->nslabs; i++) {
> - mem->slots[i].list = IO_TLB_SEGSIZE - io_tlb_offset(i);
> - mem->slots[i].orig_addr = INVALID_PHYS_ADDR;
> - mem->slots[i].alloc_size = 0;
> - }
> +
> + swiotlb_init_io_tlb_mem(mem, __pa(tlb), nslabs, false);
>
> io_tlb_default_mem = mem;
> if (verbose)
> @@ -282,8 +293,8 @@ swiotlb_late_init_with_default_size(size_t default_size)
> int
> swiotlb_late_init_with_tbl(char *tlb, unsigned long nslabs)
> {
> - unsigned long bytes = nslabs << IO_TLB_SHIFT, i;
> struct io_tlb_mem *mem;
> + unsigned long bytes = nslabs << IO_TLB_SHIFT;
>
> if (swiotlb_force == SWIOTLB_NO_FORCE)
> return 0;
> @@ -297,20 +308,9 @@ swiotlb_late_init_with_tbl(char *tlb, unsigned long
> nslabs)
> if (!mem)
> return -ENOMEM;
>
> - mem->nslabs = nslabs;
> - mem->start = virt_to_phys(tlb);
> - mem->end = mem->start + bytes;
> - mem->index = 0;
> - mem->late_alloc = 1;
> - spin_lock_init(>lock);
> - for (i = 0; i < mem->nslabs; i++) {
> - mem->slots[i].list = IO_TLB_SEGSIZE - io_tlb_offset(i);
> - mem->slots[i].orig_addr = INVALID_PHYS_ADDR;
> - mem->slots[i].alloc_size = 0;
> - }
> -
> + memset(mem, 0, sizeof(*mem));
> + swiotlb_init_io_tlb_mem(mem, virt_to_phys(tlb), nslabs, true);
> set_memory_decrypted((unsigned long)tlb, bytes >> PAGE_SHIFT);
> - memset(tlb, 0, bytes);
This is good for swiotlb_late_init_with_tbl. However I have just noticed
that mem could also be allocated from swiotlb_init_with_tbl, in which
case the zeroing is missing. I think we need another memset in
swiotlb_init_with_tbl as well. Or maybe it could be better to have a
single memset at the beginning of swiotlb_init_io_tlb_mem instead. Up to
you.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v12 06/12] swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
On Wed, 16 Jun 2021, Claire Chang wrote:
> Propagate the swiotlb_force into io_tlb_default_mem->force_bounce and
> use it to determine whether to bounce the data or not. This will be
> useful later to allow for different pools.
>
> Signed-off-by: Claire Chang
> ---
> include/linux/swiotlb.h | 11 +++
> kernel/dma/direct.c | 2 +-
> kernel/dma/direct.h | 2 +-
> kernel/dma/swiotlb.c| 4
> 4 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
> index dd1c30a83058..8d8855c77d9a 100644
> --- a/include/linux/swiotlb.h
> +++ b/include/linux/swiotlb.h
> @@ -84,6 +84,7 @@ extern enum swiotlb_force swiotlb_force;
> * unmap calls.
> * @debugfs: The dentry to debugfs.
> * @late_alloc: %true if allocated using the page allocator
> + * @force_bounce: %true if swiotlb bouncing is forced
> */
> struct io_tlb_mem {
> phys_addr_t start;
> @@ -94,6 +95,7 @@ struct io_tlb_mem {
> spinlock_t lock;
> struct dentry *debugfs;
> bool late_alloc;
> + bool force_bounce;
> struct io_tlb_slot {
> phys_addr_t orig_addr;
> size_t alloc_size;
> @@ -109,6 +111,11 @@ static inline bool is_swiotlb_buffer(struct device *dev,
> phys_addr_t paddr)
> return mem && paddr >= mem->start && paddr < mem->end;
> }
>
> +static inline bool is_swiotlb_force_bounce(struct device *dev)
> +{
> + return dev->dma_io_tlb_mem->force_bounce;
> +}
> void __init swiotlb_exit(void);
> unsigned int swiotlb_max_segment(void);
> size_t swiotlb_max_mapping_size(struct device *dev);
> @@ -120,6 +127,10 @@ static inline bool is_swiotlb_buffer(struct device *dev,
> phys_addr_t paddr)
> {
> return false;
> }
> +static inline bool is_swiotlb_force_bounce(struct device *dev)
> +{
> + return false;
> +}
> static inline void swiotlb_exit(void)
> {
> }
> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
> index 7a88c34d0867..a92465b4eb12 100644
> --- a/kernel/dma/direct.c
> +++ b/kernel/dma/direct.c
> @@ -496,7 +496,7 @@ size_t dma_direct_max_mapping_size(struct device *dev)
> {
> /* If SWIOTLB is active, use its maximum mapping size */
> if (is_swiotlb_active(dev) &&
> - (dma_addressing_limited(dev) || swiotlb_force == SWIOTLB_FORCE))
> + (dma_addressing_limited(dev) || is_swiotlb_force_bounce(dev)))
> return swiotlb_max_mapping_size(dev);
> return SIZE_MAX;
> }
> diff --git a/kernel/dma/direct.h b/kernel/dma/direct.h
> index 13e9e7158d94..4632b0f4f72e 100644
> --- a/kernel/dma/direct.h
> +++ b/kernel/dma/direct.h
> @@ -87,7 +87,7 @@ static inline dma_addr_t dma_direct_map_page(struct device
> *dev,
> phys_addr_t phys = page_to_phys(page) + offset;
> dma_addr_t dma_addr = phys_to_dma(dev, phys);
>
> - if (unlikely(swiotlb_force == SWIOTLB_FORCE))
> + if (is_swiotlb_force_bounce(dev))
> return swiotlb_map(dev, phys, size, dir, attrs);
>
> if (unlikely(!dma_capable(dev, dma_addr, size, true))) {
Should we also make the same change in
drivers/xen/swiotlb-xen.c:xen_swiotlb_map_page ?
If I make that change, I can see that everything is working as
expected for a restricted-dma device with Linux running as dom0 on Xen.
However, is_swiotlb_force_bounce returns non-zero even for normal
non-restricted-dma devices. That shouldn't happen, right?
It looks like struct io_tlb_slot is not zeroed on allocation.
Adding memset(mem, 0x0, struct_size) in swiotlb_late_init_with_tbl
solves the issue.
With those two changes, the series passes my tests and you can add my
tested-by.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v12 11/12] dt-bindings: of: Add restricted DMA pool
On Wed, 16 Jun 2021, Claire Chang wrote:
> Introduce the new compatible string, restricted-dma-pool, for restricted
> DMA. One can specify the address and length of the restricted DMA memory
> region by restricted-dma-pool in the reserved-memory node.
>
> Signed-off-by: Claire Chang
> ---
> .../reserved-memory/reserved-memory.txt | 36 +--
> 1 file changed, 33 insertions(+), 3 deletions(-)
>
> diff --git
> a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> index e8d3096d922c..46804f24df05 100644
> --- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> +++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> @@ -51,6 +51,23 @@ compatible (optional) - standard definition
>used as a shared pool of DMA buffers for a set of devices. It can
>be used by an operating system to instantiate the necessary pool
>management subsystem if necessary.
> +- restricted-dma-pool: This indicates a region of memory meant to be
> + used as a pool of restricted DMA buffers for a set of devices. The
> + memory region would be the only region accessible to those devices.
> + When using this, the no-map and reusable properties must not be
> set,
> + so the operating system can create a virtual mapping that will be
> used
> + for synchronization. The main purpose for restricted DMA is to
> + mitigate the lack of DMA access control on systems without an
> IOMMU,
> + which could result in the DMA accessing the system memory at
> + unexpected times and/or unexpected addresses, possibly leading to
> data
> + leakage or corruption. The feature on its own provides a basic
> level
> + of protection against the DMA overwriting buffer contents at
> + unexpected times. However, to protect against general data leakage
> and
> + system memory corruption, the system needs to provide way to lock
> down
> + the memory access, e.g., MPU. Note that since coherent allocation
> + needs remapping, one must set up another device coherent pool by
> + shared-dma-pool and use dma_alloc_from_dev_coherent instead for
> atomic
> + coherent allocation.
> - vendor specific string in the form ,[-]
> no-map (optional) - empty property
> - Indicates the operating system must not create a virtual mapping
> @@ -85,10 +102,11 @@ memory-region-names (optional) - a list of names, one
> for each corresponding
>
> Example
> ---
> -This example defines 3 contiguous regions are defined for Linux kernel:
> +This example defines 4 contiguous regions for Linux kernel:
> one default of all device drivers (named linux,cma@7200 and 64MiB in
> size),
> -one dedicated to the framebuffer device (named framebuffer@7800, 8MiB),
> and
> -one for multimedia processing (named multimedia-memory@7700, 64MiB).
> +one dedicated to the framebuffer device (named framebuffer@7800, 8MiB),
> +one for multimedia processing (named multimedia-memory@7700, 64MiB), and
> +one for restricted dma pool (named restricted_dma_reserved@0x5000,
> 64MiB).
>
> / {
> #address-cells = <1>;
> @@ -120,6 +138,11 @@ one for multimedia processing (named
> multimedia-memory@7700, 64MiB).
> compatible = "acme,multimedia-memory";
> reg = <0x7700 0x400>;
> };
> +
> + restricted_dma_reserved: restricted_dma_reserved {
> + compatible = "restricted-dma-pool";
> + reg = <0x5000 0x400>;
> + };
> };
>
> /* ... */
> @@ -138,4 +161,11 @@ one for multimedia processing (named
> multimedia-memory@7700, 64MiB).
> memory-region = <_reserved>;
> /* ... */
> };
> +
> + pcie_device: pcie_device@0,0 {
> + reg = <0x8301 0x0 0x 0x0 0x0010
> +0x8301 0x0 0x0010 0x0 0x0010>;
> + memory-region = <_dma_mem_reserved>;
Shouldn't it be _dma_reserved ?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: Regression when booting 5.15 as dom0 on arm64 (WAS: Re: [linux-linus test] 161829: regressions - FAIL)]
On Tue, 11 May 2021, Christoph Hellwig wrote: > On Tue, May 11, 2021 at 09:47:33AM -0700, Stefano Stabellini wrote: > > That's a much better plan. It is also not super urgent, so maybe for now > > we could add an explicit check for io_tlb_default_mem != NULL at the > > beginning of xen_swiotlb_init? So that at least we can fail explicitly > > or ignore it explicitly rather than by accident. > > Fine with me. Do you want to take over from here and test and submit > your version? I can do that. Can I add your signed-off-by for your original fix? ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: Regression when booting 5.15 as dom0 on arm64 (WAS: Re: [linux-linus test] 161829: regressions - FAIL)]
On Tue, 11 May 2021, Christoph Hellwig wrote: > On Mon, May 10, 2021 at 06:46:34PM -0700, Stefano Stabellini wrote: > > On Mon, 10 May 2021, Christoph Hellwig wrote: > > > On Sat, May 08, 2021 at 12:32:37AM +0100, Julien Grall wrote: > > > > The pointer dereferenced seems to suggest that the swiotlb hasn't been > > > > allocated. From what I can tell, this may be because swiotlb_force is > > > > set > > > > to SWIOTLB_NO_FORCE, we will still enable the swiotlb when running on > > > > top > > > > of Xen. > > > > > > > > I am not entirely sure what would be the correct fix. Any opinions? > > > > > > Can you try something like the patch below (not even compile tested, but > > > the intent should be obvious? > > > > > > > > > diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c > > > index 16a2b2b1c54d..7671bc153fb1 100644 > > > --- a/arch/arm64/mm/init.c > > > +++ b/arch/arm64/mm/init.c > > > @@ -44,6 +44,8 @@ > > > #include > > > #include > > > > > > +#include > > > + > > > /* > > > * We need to be able to catch inadvertent references to memstart_addr > > > * that occur (potentially in generic code) before arm64_memblock_init() > > > @@ -482,7 +484,7 @@ void __init mem_init(void) > > > if (swiotlb_force == SWIOTLB_FORCE || > > > max_pfn > PFN_DOWN(arm64_dma_phys_limit)) > > > swiotlb_init(1); > > > - else > > > + else if (!IS_ENABLED(CONFIG_XEN) || !xen_swiotlb_detect()) > > > swiotlb_force = SWIOTLB_NO_FORCE; > > > > > > set_max_mapnr(max_pfn - PHYS_PFN_OFFSET); > > > > The "IS_ENABLED(CONFIG_XEN)" is not needed as the check is already part > > of xen_swiotlb_detect(). > > As far as I can tell the x86 version of xen_swiotlb_detect has a > !CONFIG_XEN stub. The arm/arm64 version in uncoditionally declared, but > the implementation only compiled when Xen support is enabled. The implementation of xen_swiotlb_detect should work fine if !CONFIG_XEN, but the issue is that it is implemented in arch/arm/xen/mm.c, so it is not going to be available. I think it would be good to turn it into a static inline so that we can call it from arch/arm64/mm/init.c and other similar places with or without CONFIG_XEN, see appended patch below. It compiles without CONFIG_XEN. > > But let me ask another question first. Do you think it makes sense to have: > > > > if (swiotlb_force == SWIOTLB_NO_FORCE) > > return 0; > > > > at the beginning of swiotlb_late_init_with_tbl? I am asking because > > swiotlb_late_init_with_tbl is meant for special late initializations, > > right? It shouldn't really matter the presence or absence of > > SWIOTLB_NO_FORCE in regards to swiotlb_late_init_with_tbl. Also the > > commit message for "swiotlb: Make SWIOTLB_NO_FORCE perform no > > allocation" says that "If a platform was somehow setting > > swiotlb_no_force and a later call to swiotlb_init() was to be made we > > would still be proceeding with allocating the default SWIOTLB size > > (64MB)." Our case here is very similar, right? So the allocation should > > proceed? > > Well, right now SWIOTLB_NO_FORCE is checked in dma_direct_map_page. > We need to clean all this up a bit, especially with the work to support > multiple swiotlb buffers, but I think for now this is the best we can > do. OK > > Which brings me to a separate unrelated issue, still affecting the path > > xen_swiotlb_init -> swiotlb_late_init_with_tbl. If swiotlb_init(1) is > > called by mem_init then swiotlb_late_init_with_tbl will fail due to the > > check: > > > > /* protect against double initialization */ > > if (WARN_ON_ONCE(io_tlb_default_mem)) > > return -ENOMEM; > > > > xen_swiotlb_init is meant to ask Xen to make a bunch of pages physically > > contiguous. Then, it initializes the swiotlb buffer based on those > > pages. So it is a problem that swiotlb_late_init_with_tbl refuses to > > continue. However, in practice it is not a problem today because on ARM > > we don't actually make any special requests to Xen to make the pages > > physically contiguous (yet). See the empty implementation of > > arch/arm/xen/mm.c:xen_create_contiguous_region. I don't know about x86. > > > > So maybe we should instead do something like the appended? > > So I'd like to change the core swiotlb initialization to just use > a callback into
Re: Regression when booting 5.15 as dom0 on arm64 (WAS: Re: [linux-linus test] 161829: regressions - FAIL)
On Mon, 10 May 2021, Christoph Hellwig wrote: > On Sat, May 08, 2021 at 12:32:37AM +0100, Julien Grall wrote: > > The pointer dereferenced seems to suggest that the swiotlb hasn't been > > allocated. From what I can tell, this may be because swiotlb_force is set > > to SWIOTLB_NO_FORCE, we will still enable the swiotlb when running on top > > of Xen. > > > > I am not entirely sure what would be the correct fix. Any opinions? > > Can you try something like the patch below (not even compile tested, but > the intent should be obvious? > > > diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c > index 16a2b2b1c54d..7671bc153fb1 100644 > --- a/arch/arm64/mm/init.c > +++ b/arch/arm64/mm/init.c > @@ -44,6 +44,8 @@ > #include > #include > > +#include > + > /* > * We need to be able to catch inadvertent references to memstart_addr > * that occur (potentially in generic code) before arm64_memblock_init() > @@ -482,7 +484,7 @@ void __init mem_init(void) > if (swiotlb_force == SWIOTLB_FORCE || > max_pfn > PFN_DOWN(arm64_dma_phys_limit)) > swiotlb_init(1); > - else > + else if (!IS_ENABLED(CONFIG_XEN) || !xen_swiotlb_detect()) > swiotlb_force = SWIOTLB_NO_FORCE; > > set_max_mapnr(max_pfn - PHYS_PFN_OFFSET); The "IS_ENABLED(CONFIG_XEN)" is not needed as the check is already part of xen_swiotlb_detect(). But let me ask another question first. Do you think it makes sense to have: if (swiotlb_force == SWIOTLB_NO_FORCE) return 0; at the beginning of swiotlb_late_init_with_tbl? I am asking because swiotlb_late_init_with_tbl is meant for special late initializations, right? It shouldn't really matter the presence or absence of SWIOTLB_NO_FORCE in regards to swiotlb_late_init_with_tbl. Also the commit message for "swiotlb: Make SWIOTLB_NO_FORCE perform no allocation" says that "If a platform was somehow setting swiotlb_no_force and a later call to swiotlb_init() was to be made we would still be proceeding with allocating the default SWIOTLB size (64MB)." Our case here is very similar, right? So the allocation should proceed? Which brings me to a separate unrelated issue, still affecting the path xen_swiotlb_init -> swiotlb_late_init_with_tbl. If swiotlb_init(1) is called by mem_init then swiotlb_late_init_with_tbl will fail due to the check: /* protect against double initialization */ if (WARN_ON_ONCE(io_tlb_default_mem)) return -ENOMEM; xen_swiotlb_init is meant to ask Xen to make a bunch of pages physically contiguous. Then, it initializes the swiotlb buffer based on those pages. So it is a problem that swiotlb_late_init_with_tbl refuses to continue. However, in practice it is not a problem today because on ARM we don't actually make any special requests to Xen to make the pages physically contiguous (yet). See the empty implementation of arch/arm/xen/mm.c:xen_create_contiguous_region. I don't know about x86. So maybe we should instead do something like the appended? diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c index f8f07469d259..f5a3638d1dee 100644 --- a/arch/arm/xen/mm.c +++ b/arch/arm/xen/mm.c @@ -152,6 +152,7 @@ static int __init xen_mm_init(void) struct gnttab_cache_flush cflush; if (!xen_swiotlb_detect()) return 0; + swiotlb_exit(); xen_swiotlb_init(); cflush.op = 0; diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c index 8ca7d505d61c..f17be37298a7 100644 --- a/kernel/dma/swiotlb.c +++ b/kernel/dma/swiotlb.c @@ -285,9 +285,6 @@ swiotlb_late_init_with_tbl(char *tlb, unsigned long nslabs) unsigned long bytes = nslabs << IO_TLB_SHIFT, i; struct io_tlb_mem *mem; - if (swiotlb_force == SWIOTLB_NO_FORCE) - return 0; - /* protect against double initialization */ if (WARN_ON_ONCE(io_tlb_default_mem)) return -ENOMEM; ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] ARM: Qualify enabling of swiotlb_init()
On Fri, 19 Mar 2021, Konrad Rzeszutek Wilk wrote: > On Fri, Mar 19, 2021 at 02:07:31PM +0100, Christoph Hellwig wrote: > > On Thu, Mar 18, 2021 at 09:03:33PM -0700, Florian Fainelli wrote: > > > #ifdef CONFIG_ARM_LPAE > > > + if (swiotlb_force == SWIOTLB_FORCE || > > > + max_pfn > arm_dma_pfn_limit) > > > > Does arm_dma_pfn_limit do the right thing even with the weirdest > > remapping ranges? Maybe a commen here would be useful. > > > > > + swiotlb_init(1); > > > + else > > > + swiotlb_force = SWIOTLB_NO_FORCE; > > > > Konrad: what do you think of setting swiotlb_force to SWIOTLB_NO_FORCE > > and only switching it to SWIOTLB_NORMAL when swiotlb_init* is called? > > That kind makes more sense than forcing the callers to do it. > > > > While we're at it, I think swiotlb_force should probably be renamed to > > swiotlb_mode or somethng like that. > > swiotlb_mode sounds good. > > Also it got me thinking - ARM on Xen at some point was a bit strange, so not > sure how > the logic works here, Stefano? There is nothing strange in regards to swiotlb_force. swiotlb_force is only used in swiotlb-xen map_page to figure out whether: - we actually have to use the swiotlb bounce buffer (this is the swiotlb_xen == SWIOTLB_FORCE case) - or we can use the provided page directly for dma if other conditions are met (dma_capable, !range_straddles_page_boundary, ...) I don't think that switching to "swiotlb_mode" would cause any issues. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC v1 5/6] xen-swiotlb: convert variables to arrays
On Fri, 19 Feb 2021, Konrad Rzeszutek Wilk wrote:
> On Sun, Feb 07, 2021 at 04:56:01PM +0100, Christoph Hellwig wrote:
> > On Thu, Feb 04, 2021 at 09:40:23AM +0100, Christoph Hellwig wrote:
> > > So one thing that has been on my mind for a while: I'd really like
> > > to kill the separate dma ops in Xen swiotlb. If we compare xen-swiotlb
> > > to swiotlb the main difference seems to be:
> > >
> > > - additional reasons to bounce I/O vs the plain DMA capable
> > > - the possibility to do a hypercall on arm/arm64
> > > - an extra translation layer before doing the phys_to_dma and vice
> > >versa
> > > - an special memory allocator
> > >
> > > I wonder if inbetween a few jump labels or other no overhead enablement
> > > options and possibly better use of the dma_range_map we could kill
> > > off most of swiotlb-xen instead of maintaining all this code duplication?
> >
> > So I looked at this a bit more.
> >
> > For x86 with XENFEAT_auto_translated_physmap (how common is that?)
>
> Juergen, Boris please correct me if I am wrong, but that
> XENFEAT_auto_translated_physmap
> only works for PVH guests?
ARM is always XENFEAT_auto_translated_physmap
> > pfn_to_gfn is a nop, so plain phys_to_dma/dma_to_phys do work as-is.
> >
> > xen_arch_need_swiotlb always returns true for x86, and
> > range_straddles_page_boundary should never be true for the
> > XENFEAT_auto_translated_physmap case.
>
> Correct. The kernel should have no clue of what the real MFNs are
> for PFNs.
On ARM, Linux knows the MFNs because for local pages MFN == PFN and for
foreign pages it keeps track in arch/arm/xen/p2m.c. More on this below.
xen_arch_need_swiotlb only returns true on ARM in rare situations where
bouncing on swiotlb buffers is required. Today it only happens on old
versions of Xen that don't support the cache flushing hypercall but
there could be more cases in the future.
> >
> > So as far as I can tell the mapping fast path for the
> > XENFEAT_auto_translated_physmap can be trivially reused from swiotlb.
> >
> > That leaves us with the next more complicated case, x86 or fully cache
> > coherent arm{,64} without XENFEAT_auto_translated_physmap. In that case
> > we need to patch in a phys_to_dma/dma_to_phys that performs the MFN
> > lookup, which could be done using alternatives or jump labels.
> > I think if that is done right we should also be able to let that cover
> > the foreign pages in is_xen_swiotlb_buffer/is_swiotlb_buffer, but
> > in that worst case that would need another alternative / jump label.
> >
> > For non-coherent arm{,64} we'd also need to use alternatives or jump
> > labels to for the cache maintainance ops, but that isn't a hard problem
> > either.
With the caveat that ARM is always XENFEAT_auto_translated_physmap, what
you wrote looks correct. I am writing down a brief explanation on how
swiotlb-xen is used on ARM.
pfn: address as seen by the guest, pseudo-physical address in ARM terminology
mfn (or bfn): real address, physical address in ARM terminology
On ARM dom0 is auto_translated (so Xen sets up the stage2 translation
in the MMU) and the translation is 1:1. So pfn == mfn for Dom0.
However, when another domain shares a page with Dom0, that page is not
1:1. Swiotlb-xen is used to retrieve the mfn for the foreign page at
xen_swiotlb_map_page. It does that with xen_phys_to_bus -> pfn_to_bfn.
It is implemented with a rbtree in arch/arm/xen/p2m.c.
In addition, swiotlb-xen is also used to cache-flush the page via
hypercall at xen_swiotlb_unmap_page. That is done because dev_addr is
really the mfn at unmap_page and we don't know the pfn for it. We can do
pfn-to-mfn but we cannot do mfn-to-pfn (there are good reasons for it
unfortunately). The only way to cache-flush by mfn is by issuing a
hypercall. The hypercall is implemented in arch/arm/xen/mm.c.
The pfn != bfn and pfn_valid() checks are used to detect if the page is
local (of dom0) or foreign; they work thanks to the fact that Dom0 is
1:1 mapped.
Getting back to what you wrote, yes if we had a way to do MFN lookups in
phys_to_dma, and a way to call the hypercall at unmap_page if the page
is foreign (e.g. if it fails a pfn_valid check) then I think we would be
good from an ARM perspective. The only exception is when
xen_arch_need_swiotlb returns true, in which case we need to actually
bounce on swiotlb buffers.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] fix swiotlb panic on Xen
On Tue, 27 Oct 2020, Konrad Rzeszutek Wilk wrote:
> > As the person who first found this and then confirmed this fixes a bug:
> >
> > Tested-by: Elliott Mitchell
>
> Thank you!!
>
> I changed the title and added the various tags and will put it in
> linux-next later this week.
Looks fine, thank you
> >From a1eb2768bf5954d25aa0f0136b38f0aa5d92d984 Mon Sep 17 00:00:00 2001
> From: Stefano Stabellini
> Date: Mon, 26 Oct 2020 17:02:14 -0700
> Subject: [PATCH] swiotlb: fix "x86: Don't panic if can not alloc buffer for
> swiotlb"
>
> kernel/dma/swiotlb.c:swiotlb_init gets called first and tries to
> allocate a buffer for the swiotlb. It does so by calling
>
> memblock_alloc_low(PAGE_ALIGN(bytes), PAGE_SIZE);
>
> If the allocation must fail, no_iotlb_memory is set.
>
> Later during initialization swiotlb-xen comes in
> (drivers/xen/swiotlb-xen.c:xen_swiotlb_init) and given that io_tlb_start
> is != 0, it thinks the memory is ready to use when actually it is not.
>
> When the swiotlb is actually needed, swiotlb_tbl_map_single gets called
> and since no_iotlb_memory is set the kernel panics.
>
> Instead, if swiotlb-xen.c:xen_swiotlb_init knew the swiotlb hadn't been
> initialized, it would do the initialization itself, which might still
> succeed.
>
> Fix the panic by setting io_tlb_start to 0 on swiotlb initialization
> failure, and also by setting no_iotlb_memory to false on swiotlb
> initialization success.
>
> Fixes: ac2cbab21f31 ("x86: Don't panic if can not alloc buffer for swiotlb")
>
> Reported-by: Elliott Mitchell
> Tested-by: Elliott Mitchell
> Signed-off-by: Stefano Stabellini
> Reviewed-by: Christoph Hellwig
> CC: sta...@vger.kernel.org
> Signed-off-by: Konrad Rzeszutek Wilk
> ---
> kernel/dma/swiotlb.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index 465a567678d9..e08cac39c0ba 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -229,6 +229,7 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned long
> nslabs, int verbose)
> io_tlb_orig_addr[i] = INVALID_PHYS_ADDR;
> }
> io_tlb_index = 0;
> + no_iotlb_memory = false;
>
> if (verbose)
> swiotlb_print_info();
> @@ -260,9 +261,11 @@ swiotlb_init(int verbose)
> if (vstart && !swiotlb_init_with_tbl(vstart, io_tlb_nslabs, verbose))
> return;
>
> - if (io_tlb_start)
> + if (io_tlb_start) {
> memblock_free_early(io_tlb_start,
> PAGE_ALIGN(io_tlb_nslabs << IO_TLB_SHIFT));
> + io_tlb_start = 0;
> + }
> pr_warn("Cannot allocate buffer");
> no_iotlb_memory = true;
> }
> @@ -360,6 +363,7 @@ swiotlb_late_init_with_tbl(char *tlb, unsigned long
> nslabs)
> io_tlb_orig_addr[i] = INVALID_PHYS_ADDR;
> }
> io_tlb_index = 0;
> + no_iotlb_memory = false;
>
> swiotlb_print_info();
>
> --
> 2.13.6
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] fix swiotlb panic on Xen
On Tue, 27 Oct 2020, Konrad Rzeszutek Wilk wrote:
> On Mon, Oct 26, 2020 at 05:02:14PM -0700, Stefano Stabellini wrote:
> > From: Stefano Stabellini
> >
> > kernel/dma/swiotlb.c:swiotlb_init gets called first and tries to
> > allocate a buffer for the swiotlb. It does so by calling
> >
> > memblock_alloc_low(PAGE_ALIGN(bytes), PAGE_SIZE);
> >
> > If the allocation must fail, no_iotlb_memory is set.
> >
> >
> > Later during initialization swiotlb-xen comes in
> > (drivers/xen/swiotlb-xen.c:xen_swiotlb_init) and given that io_tlb_start
> > is != 0, it thinks the memory is ready to use when actually it is not.
> >
> > When the swiotlb is actually needed, swiotlb_tbl_map_single gets called
> > and since no_iotlb_memory is set the kernel panics.
> >
> > Instead, if swiotlb-xen.c:xen_swiotlb_init knew the swiotlb hadn't been
> > initialized, it would do the initialization itself, which might still
> > succeed.
> >
> >
> > Fix the panic by setting io_tlb_start to 0 on swiotlb initialization
> > failure, and also by setting no_iotlb_memory to false on swiotlb
> > initialization success.
>
> Should this have a Fixes: flag?
That would be
Fixes: ac2cbab21f31 ("x86: Don't panic if can not alloc buffer for swiotlb")
> > Signed-off-by: Stefano Stabellini
> >
> >
> > diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> > index c19379fabd20..9924214df60a 100644
> > --- a/kernel/dma/swiotlb.c
> > +++ b/kernel/dma/swiotlb.c
> > @@ -231,6 +231,7 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned
> > long nslabs, int verbose)
> > io_tlb_orig_addr[i] = INVALID_PHYS_ADDR;
> > }
> > io_tlb_index = 0;
> > + no_iotlb_memory = false;
> >
> > if (verbose)
> > swiotlb_print_info();
> > @@ -262,9 +263,11 @@ swiotlb_init(int verbose)
> > if (vstart && !swiotlb_init_with_tbl(vstart, io_tlb_nslabs, verbose))
> > return;
> >
> > - if (io_tlb_start)
> > + if (io_tlb_start) {
> > memblock_free_early(io_tlb_start,
> > PAGE_ALIGN(io_tlb_nslabs << IO_TLB_SHIFT));
> > + io_tlb_start = 0;
> > + }
> > pr_warn("Cannot allocate buffer");
> > no_iotlb_memory = true;
> > }
> > @@ -362,6 +365,7 @@ swiotlb_late_init_with_tbl(char *tlb, unsigned long
> > nslabs)
> > io_tlb_orig_addr[i] = INVALID_PHYS_ADDR;
> > }
> > io_tlb_index = 0;
> > + no_iotlb_memory = false;
> >
> > swiotlb_print_info();
> >
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] fix swiotlb panic on Xen
From: Stefano Stabellini
kernel/dma/swiotlb.c:swiotlb_init gets called first and tries to
allocate a buffer for the swiotlb. It does so by calling
memblock_alloc_low(PAGE_ALIGN(bytes), PAGE_SIZE);
If the allocation must fail, no_iotlb_memory is set.
Later during initialization swiotlb-xen comes in
(drivers/xen/swiotlb-xen.c:xen_swiotlb_init) and given that io_tlb_start
is != 0, it thinks the memory is ready to use when actually it is not.
When the swiotlb is actually needed, swiotlb_tbl_map_single gets called
and since no_iotlb_memory is set the kernel panics.
Instead, if swiotlb-xen.c:xen_swiotlb_init knew the swiotlb hadn't been
initialized, it would do the initialization itself, which might still
succeed.
Fix the panic by setting io_tlb_start to 0 on swiotlb initialization
failure, and also by setting no_iotlb_memory to false on swiotlb
initialization success.
Signed-off-by: Stefano Stabellini
diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index c19379fabd20..9924214df60a 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -231,6 +231,7 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned long
nslabs, int verbose)
io_tlb_orig_addr[i] = INVALID_PHYS_ADDR;
}
io_tlb_index = 0;
+ no_iotlb_memory = false;
if (verbose)
swiotlb_print_info();
@@ -262,9 +263,11 @@ swiotlb_init(int verbose)
if (vstart && !swiotlb_init_with_tbl(vstart, io_tlb_nslabs, verbose))
return;
- if (io_tlb_start)
+ if (io_tlb_start) {
memblock_free_early(io_tlb_start,
PAGE_ALIGN(io_tlb_nslabs << IO_TLB_SHIFT));
+ io_tlb_start = 0;
+ }
pr_warn("Cannot allocate buffer");
no_iotlb_memory = true;
}
@@ -362,6 +365,7 @@ swiotlb_late_init_with_tbl(char *tlb, unsigned long nslabs)
io_tlb_orig_addr[i] = INVALID_PHYS_ADDR;
}
io_tlb_index = 0;
+ no_iotlb_memory = false;
swiotlb_print_info();
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
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, 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=159011972107683=2
> > https://marc.info/?l=linux-kernel=159018047129198=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
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=159011972107683=2 https://marc.info/?l=linux-kernel=159018047129198=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
Re: [PATCH] xen: introduce xen_vring_use_dma
On Sat, 11 Jul 2020, Michael S. Tsirkin wrote: > On Fri, Jul 10, 2020 at 10:23:22AM -0700, Stefano Stabellini wrote: > > Sorry for the late reply -- a couple of conferences kept me busy. > > > > > > On Wed, 1 Jul 2020, Michael S. Tsirkin wrote: > > > On Wed, Jul 01, 2020 at 10:34:53AM -0700, Stefano Stabellini wrote: > > > > Would you be in favor of a more flexible check along the lines of the > > > > one proposed in the patch that started this thread: > > > > > > > > if (xen_vring_use_dma()) > > > > return true; > > > > > > > > > > > > xen_vring_use_dma would be implemented so that it returns true when > > > > xen_swiotlb is required and false otherwise. > > > > > > Just to stress - with a patch like this virtio can *still* use DMA API > > > if PLATFORM_ACCESS is set. So if DMA API is broken on some platforms > > > as you seem to be saying, you guys should fix it before doing something > > > like this.. > > > > Yes, DMA API is broken with some interfaces (specifically: rpmesg and > > trusty), but for them PLATFORM_ACCESS is never set. That is why the > > errors weren't reported before. Xen special case aside, there is no > > problem under normal circumstances. > > So why not fix DMA API? Then this patch is not needed. It looks like the conversation is going in circles :-) I tried to explain the reason why, even if we fixed the DMA API to work with rpmesg and trusty, we still need this patch with the following email: https://marc.info/?l=linux-kernel=159347446709625=2 At that time it looked like you agreed that we needed to improve this check? (https://marc.info/?l=linux-kernel=159363662418250=2) ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] xen: introduce xen_vring_use_dma
Sorry for the late reply -- a couple of conferences kept me busy. On Wed, 1 Jul 2020, Michael S. Tsirkin wrote: > On Wed, Jul 01, 2020 at 10:34:53AM -0700, Stefano Stabellini wrote: > > Would you be in favor of a more flexible check along the lines of the > > one proposed in the patch that started this thread: > > > > if (xen_vring_use_dma()) > > return true; > > > > > > xen_vring_use_dma would be implemented so that it returns true when > > xen_swiotlb is required and false otherwise. > > Just to stress - with a patch like this virtio can *still* use DMA API > if PLATFORM_ACCESS is set. So if DMA API is broken on some platforms > as you seem to be saying, you guys should fix it before doing something > like this.. Yes, DMA API is broken with some interfaces (specifically: rpmesg and trusty), but for them PLATFORM_ACCESS is never set. That is why the errors weren't reported before. Xen special case aside, there is no problem under normal circumstances. If you are OK with this patch (after a little bit of clean-up), Peng, are you OK with sending an update or do you want me to? ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] xen: introduce xen_vring_use_dma
On Wed, 1 Jul 2020, Christoph Hellwig wrote: > On Mon, Jun 29, 2020 at 04:46:09PM -0700, Stefano Stabellini wrote: > > > I could imagine some future Xen hosts setting a flag somewhere in the > > > platform capability saying "no xen specific flag, rely on > > > "VIRTIO_F_ACCESS_PLATFORM". Then you set that accordingly in QEMU. > > > How about that? > > > > Yes, that would be fine and there is no problem implementing something > > like that when we get virtio support in Xen. Today there are still no > > virtio interfaces provided by Xen to ARM guests (no virtio-block/net, > > etc.) > > > > In fact, in both cases we are discussing virtio is *not* provided by > > Xen; it is a firmware interface to something entirely different: > > > > 1) virtio is used to talk to a remote AMP processor (RPMesg) > > 2) virtio is used to talk to a secure-world firmware/OS (Trusty) > > > > VIRTIO_F_ACCESS_PLATFORM is not set by Xen in these cases but by RPMesg > > and by Trusty respectively. I don't know if Trusty should or should not > > set VIRTIO_F_ACCESS_PLATFORM, but I think Linux should still work > > without issues. > > > > Any virtio implementation that is not in control of the memory map > (aka not the hypervisor) absolutely must set VIRTIO_F_ACCESS_PLATFORM, > else it is completely broken. Lots of broken virtio implementations out there it would seem :-( > > The xen_domain() check in Linux makes it so that vring_use_dma_api > > returns the opposite value on native Linux compared to Linux as Xen/ARM > > DomU by "accident". By "accident" because there is no architectural > > reason why Linux Xen/ARM DomU should behave differently compared to > > native Linux in this regard. > > > > I hope that now it is clearer why I think the if (xen_domain()) check > > needs to be improved anyway, even if we fix generic dma_ops with virtio > > interfaces missing VIRTIO_F_ACCESS_PLATFORM. > > IMHO that Xen quirk should never have been added in this form.. Would you be in favor of a more flexible check along the lines of the one proposed in the patch that started this thread: if (xen_vring_use_dma()) return true; xen_vring_use_dma would be implemented so that it returns true when xen_swiotlb is required and false otherwise. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH] xen: introduce xen_vring_use_dma
On Mon, 29 Jun 2020, Peng Fan wrote: > > > If that is the case, how is it possible that virtio breaks on ARM > > > using the default dma_ops? The breakage is not Xen related (except > > > that Xen turns dma_ops on). The original message from Peng was: > > > > > > vring_map_one_sg -> vring_use_dma_api > > >-> dma_map_page > > > -> __swiotlb_map_page > > > ->swiotlb_map_page > > > > > > ->__dma_map_area(phys_to_virt(dma_to_phys(dev, > > dev_addr)), size, dir); > > > However we are using per device dma area for rpmsg, phys_to_virt > > > could not return a correct virtual address for virtual address in > > > vmalloc area. Then kernel panic. > > > > > > I must be missing something. Maybe it is because it has to do with RPMesg? > > > > I think it's an RPMesg bug, yes > > rpmsg bug is another issue, it should not use dma_alloc_coherent for reserved > area, > and use vmalloc_to_page. > > Anyway here using dma api will also trigger issue. Is the stack trace above for the RPMesg issue or for the Trusty issue? If it is the stack trace for RPMesg, can you also post the Trusty stack trace? Or are they indentical? ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] xen: introduce xen_vring_use_dma
On Fri, 26 Jun 2020, Michael S. Tsirkin wrote: > On Thu, Jun 25, 2020 at 10:31:27AM -0700, Stefano Stabellini wrote: > > On Wed, 24 Jun 2020, Michael S. Tsirkin wrote: > > > On Wed, Jun 24, 2020 at 02:53:54PM -0700, Stefano Stabellini wrote: > > > > On Wed, 24 Jun 2020, Michael S. Tsirkin wrote: > > > > > On Wed, Jun 24, 2020 at 10:59:47AM -0700, Stefano Stabellini wrote: > > > > > > On Wed, 24 Jun 2020, Michael S. Tsirkin wrote: > > > > > > > On Wed, Jun 24, 2020 at 05:17:32PM +0800, Peng Fan wrote: > > > > > > > > Export xen_swiotlb for all platforms using xen swiotlb > > > > > > > > > > > > > > > > Use xen_swiotlb to determine when vring should use dma APIs to > > > > > > > > map the > > > > > > > > ring: when xen_swiotlb is enabled the dma API is required. When > > > > > > > > it is > > > > > > > > disabled, it is not required. > > > > > > > > > > > > > > > > Signed-off-by: Peng Fan > > > > > > > > > > > > > > Isn't there some way to use VIRTIO_F_IOMMU_PLATFORM for this? > > > > > > > Xen was there first, but everyone else is using that now. > > > > > > > > > > > > Unfortunately it is complicated and it is not related to > > > > > > VIRTIO_F_IOMMU_PLATFORM :-( > > > > > > > > > > > > > > > > > > The Xen subsystem in Linux uses dma_ops via swiotlb_xen to translate > > > > > > foreign mappings (memory coming from other VMs) to physical > > > > > > addresses. > > > > > > On x86, it also uses dma_ops to translate Linux's idea of a physical > > > > > > address into a real physical address (this is unneeded on ARM.) > > > > > > > > > > > > > > > > > > So regardless of VIRTIO_F_IOMMU_PLATFORM, dma_ops should be used on > > > > > > Xen/x86 > > > > > > always and on Xen/ARM if Linux is Dom0 (because it has foreign > > > > > > mappings.) That is why we have the if (xen_domain) return true; in > > > > > > vring_use_dma_api. > > > > > > > > > > VIRTIO_F_IOMMU_PLATFORM makes guest always use DMA ops. > > > > > > > > > > Xen hack predates VIRTIO_F_IOMMU_PLATFORM so it *also* > > > > > forces DMA ops even if VIRTIO_F_IOMMU_PLATFORM is clear. > > > > > > > > > > Unfortunately as a result Xen never got around to > > > > > properly setting VIRTIO_F_IOMMU_PLATFORM. > > > > > > > > I don't think VIRTIO_F_IOMMU_PLATFORM would be correct for this because > > > > the usage of swiotlb_xen is not a property of virtio, > > > > > > > > > Basically any device without VIRTIO_F_ACCESS_PLATFORM > > > (that is it's name in latest virtio spec, VIRTIO_F_IOMMU_PLATFORM is > > > what linux calls it) is declared as "special, don't follow normal rules > > > for access". > > > > > > So yes swiotlb_xen is not a property of virtio, but what *is* a property > > > of virtio is that it's not special, just a regular device from DMA POV. > > > > I am trying to understand what you meant but I think I am missing > > something. > > > > Are you saying that modern virtio should always have > > VIRTIO_F_ACCESS_PLATFORM, hence use normal dma_ops as any other devices? > > I am saying it's a safe default. Clear VIRTIO_F_ACCESS_PLATFORM if you > have some special needs e.g. you are very sure it's ok to bypass DMA > ops, or you need to support a legacy guest (produced in the window > between virtio 1 support in 2014 and support for > VIRTIO_F_ACCESS_PLATFORM in 2016). Ok thanks. I understand and it makes sense to me. > > > > > > You might have noticed that I missed one possible case above: > > > > > > Xen/ARM > > > > > > DomU :-) > > > > > > > > > > > > Xen/ARM domUs don't need swiotlb_xen, it is not even initialized. > > > > > > So if > > > > > > (xen_domain) return true; would give the wrong answer in that case. > > > > > > Linux would end up calling the "normal" dma_ops, not swiotlb-xen, > > > > > > and > > > > > > the "normal" dma_ops fail. > > > > > > >
Re: [PATCH] xen: introduce xen_vring_use_dma
On Wed, 24 Jun 2020, Michael S. Tsirkin wrote: > On Wed, Jun 24, 2020 at 02:53:54PM -0700, Stefano Stabellini wrote: > > On Wed, 24 Jun 2020, Michael S. Tsirkin wrote: > > > On Wed, Jun 24, 2020 at 10:59:47AM -0700, Stefano Stabellini wrote: > > > > On Wed, 24 Jun 2020, Michael S. Tsirkin wrote: > > > > > On Wed, Jun 24, 2020 at 05:17:32PM +0800, Peng Fan wrote: > > > > > > Export xen_swiotlb for all platforms using xen swiotlb > > > > > > > > > > > > Use xen_swiotlb to determine when vring should use dma APIs to map > > > > > > the > > > > > > ring: when xen_swiotlb is enabled the dma API is required. When it > > > > > > is > > > > > > disabled, it is not required. > > > > > > > > > > > > Signed-off-by: Peng Fan > > > > > > > > > > Isn't there some way to use VIRTIO_F_IOMMU_PLATFORM for this? > > > > > Xen was there first, but everyone else is using that now. > > > > > > > > Unfortunately it is complicated and it is not related to > > > > VIRTIO_F_IOMMU_PLATFORM :-( > > > > > > > > > > > > The Xen subsystem in Linux uses dma_ops via swiotlb_xen to translate > > > > foreign mappings (memory coming from other VMs) to physical addresses. > > > > On x86, it also uses dma_ops to translate Linux's idea of a physical > > > > address into a real physical address (this is unneeded on ARM.) > > > > > > > > > > > > So regardless of VIRTIO_F_IOMMU_PLATFORM, dma_ops should be used on > > > > Xen/x86 > > > > always and on Xen/ARM if Linux is Dom0 (because it has foreign > > > > mappings.) That is why we have the if (xen_domain) return true; in > > > > vring_use_dma_api. > > > > > > VIRTIO_F_IOMMU_PLATFORM makes guest always use DMA ops. > > > > > > Xen hack predates VIRTIO_F_IOMMU_PLATFORM so it *also* > > > forces DMA ops even if VIRTIO_F_IOMMU_PLATFORM is clear. > > > > > > Unfortunately as a result Xen never got around to > > > properly setting VIRTIO_F_IOMMU_PLATFORM. > > > > I don't think VIRTIO_F_IOMMU_PLATFORM would be correct for this because > > the usage of swiotlb_xen is not a property of virtio, > > > Basically any device without VIRTIO_F_ACCESS_PLATFORM > (that is it's name in latest virtio spec, VIRTIO_F_IOMMU_PLATFORM is > what linux calls it) is declared as "special, don't follow normal rules > for access". > > So yes swiotlb_xen is not a property of virtio, but what *is* a property > of virtio is that it's not special, just a regular device from DMA POV. I am trying to understand what you meant but I think I am missing something. Are you saying that modern virtio should always have VIRTIO_F_ACCESS_PLATFORM, hence use normal dma_ops as any other devices? If that is the case, how is it possible that virtio breaks on ARM using the default dma_ops? The breakage is not Xen related (except that Xen turns dma_ops on). The original message from Peng was: vring_map_one_sg -> vring_use_dma_api -> dma_map_page -> __swiotlb_map_page ->swiotlb_map_page ->__dma_map_area(phys_to_virt(dma_to_phys(dev, dev_addr)), size, dir); However we are using per device dma area for rpmsg, phys_to_virt could not return a correct virtual address for virtual address in vmalloc area. Then kernel panic. I must be missing something. Maybe it is because it has to do with RPMesg? > > > > You might have noticed that I missed one possible case above: Xen/ARM > > > > DomU :-) > > > > > > > > Xen/ARM domUs don't need swiotlb_xen, it is not even initialized. So if > > > > (xen_domain) return true; would give the wrong answer in that case. > > > > Linux would end up calling the "normal" dma_ops, not swiotlb-xen, and > > > > the "normal" dma_ops fail. > > > > > > > > > > > > The solution I suggested was to make the check in vring_use_dma_api more > > > > flexible by returning true if the swiotlb_xen is supposed to be used, > > > > not in general for all Xen domains, because that is what the check was > > > > really meant to do. > > > > > > Why not fix DMA ops so they DTRT (nop) on Xen/ARM DomU? What is wrong > > > with that? > > > > swiotlb-xen is not used on Xen/ARM DomU, the default dma_ops are the > >
Re: [PATCH] xen: introduce xen_vring_use_dma
On Wed, 24 Jun 2020, Michael S. Tsirkin wrote: > On Wed, Jun 24, 2020 at 10:59:47AM -0700, Stefano Stabellini wrote: > > On Wed, 24 Jun 2020, Michael S. Tsirkin wrote: > > > On Wed, Jun 24, 2020 at 05:17:32PM +0800, Peng Fan wrote: > > > > Export xen_swiotlb for all platforms using xen swiotlb > > > > > > > > Use xen_swiotlb to determine when vring should use dma APIs to map the > > > > ring: when xen_swiotlb is enabled the dma API is required. When it is > > > > disabled, it is not required. > > > > > > > > Signed-off-by: Peng Fan > > > > > > Isn't there some way to use VIRTIO_F_IOMMU_PLATFORM for this? > > > Xen was there first, but everyone else is using that now. > > > > Unfortunately it is complicated and it is not related to > > VIRTIO_F_IOMMU_PLATFORM :-( > > > > > > The Xen subsystem in Linux uses dma_ops via swiotlb_xen to translate > > foreign mappings (memory coming from other VMs) to physical addresses. > > On x86, it also uses dma_ops to translate Linux's idea of a physical > > address into a real physical address (this is unneeded on ARM.) > > > > > > So regardless of VIRTIO_F_IOMMU_PLATFORM, dma_ops should be used on Xen/x86 > > always and on Xen/ARM if Linux is Dom0 (because it has foreign > > mappings.) That is why we have the if (xen_domain) return true; in > > vring_use_dma_api. > > VIRTIO_F_IOMMU_PLATFORM makes guest always use DMA ops. > > Xen hack predates VIRTIO_F_IOMMU_PLATFORM so it *also* > forces DMA ops even if VIRTIO_F_IOMMU_PLATFORM is clear. > > Unfortunately as a result Xen never got around to > properly setting VIRTIO_F_IOMMU_PLATFORM. I don't think VIRTIO_F_IOMMU_PLATFORM would be correct for this because the usage of swiotlb_xen is not a property of virtio, it is a detail of the way Linux does Xen address translations. swiotlb-xen is used to do these translations and it is hooked into the dma_ops framework. It would be possible to have a device in hardware that is virtio-compatible and doesn't set VIRTIO_F_IOMMU_PLATFORM. The device could be directly assigned (passthrough) to a DomU. We would still have to use swiotlb_xen if Xen is running. You should think of swiotlb-xen as only internal to Linux and not related to whether the (virtual or non-virtual) hardware comes with an IOMMU or not. > > You might have noticed that I missed one possible case above: Xen/ARM > > DomU :-) > > > > Xen/ARM domUs don't need swiotlb_xen, it is not even initialized. So if > > (xen_domain) return true; would give the wrong answer in that case. > > Linux would end up calling the "normal" dma_ops, not swiotlb-xen, and > > the "normal" dma_ops fail. > > > > > > The solution I suggested was to make the check in vring_use_dma_api more > > flexible by returning true if the swiotlb_xen is supposed to be used, > > not in general for all Xen domains, because that is what the check was > > really meant to do. > > Why not fix DMA ops so they DTRT (nop) on Xen/ARM DomU? What is wrong with > that? swiotlb-xen is not used on Xen/ARM DomU, the default dma_ops are the ones that are used. So you are saying, why don't we fix the default dma_ops to work with virtio? It is bad that the default dma_ops crash with virtio, so yes I think it would be good to fix that. However, even if we fixed that, the if (xen_domain()) check in vring_use_dma_api is still a problem. Alternatively we could try to work-around it from swiotlb-xen. We could enable swiotlb-xen for Xen/ARM DomUs with a different implementation so that we could leave the vring_use_dma_api check unmodified. It would be ugly because we would have to figure out from the new swiotlb-xen functions if the device is a normal device, so we have to call the regular dma_ops functions, or if the device is a virtio device, in which case there is nothing to do. I think it is undesirable but could probably be made to work. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] xen: introduce xen_vring_use_dma
On Wed, 24 Jun 2020, Michael S. Tsirkin wrote: > On Wed, Jun 24, 2020 at 05:17:32PM +0800, Peng Fan wrote: > > Export xen_swiotlb for all platforms using xen swiotlb > > > > Use xen_swiotlb to determine when vring should use dma APIs to map the > > ring: when xen_swiotlb is enabled the dma API is required. When it is > > disabled, it is not required. > > > > Signed-off-by: Peng Fan > > Isn't there some way to use VIRTIO_F_IOMMU_PLATFORM for this? > Xen was there first, but everyone else is using that now. Unfortunately it is complicated and it is not related to VIRTIO_F_IOMMU_PLATFORM :-( The Xen subsystem in Linux uses dma_ops via swiotlb_xen to translate foreign mappings (memory coming from other VMs) to physical addresses. On x86, it also uses dma_ops to translate Linux's idea of a physical address into a real physical address (this is unneeded on ARM.) So regardless of VIRTIO_F_IOMMU_PLATFORM, dma_ops should be used on Xen/x86 always and on Xen/ARM if Linux is Dom0 (because it has foreign mappings.) That is why we have the if (xen_domain) return true; in vring_use_dma_api. You might have noticed that I missed one possible case above: Xen/ARM DomU :-) Xen/ARM domUs don't need swiotlb_xen, it is not even initialized. So if (xen_domain) return true; would give the wrong answer in that case. Linux would end up calling the "normal" dma_ops, not swiotlb-xen, and the "normal" dma_ops fail. The solution I suggested was to make the check in vring_use_dma_api more flexible by returning true if the swiotlb_xen is supposed to be used, not in general for all Xen domains, because that is what the check was really meant to do. In this regards I have two more comments: - the comment on top of the check in vring_use_dma_api is still valid - the patch looks broken on x86: it should always return true, but it returns false instead > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > index a2de775801af..768afd79f67a 100644 > > --- a/drivers/virtio/virtio_ring.c > > +++ b/drivers/virtio/virtio_ring.c > > @@ -252,7 +252,7 @@ static bool vring_use_dma_api(struct virtio_device > > *vdev) > > * the DMA API if we're a Xen guest, which at least allows > > * all of the sensible Xen configurations to work correctly. > > */ > > - if (xen_domain()) > > + if (xen_vring_use_dma()) > > return true; > > > > return false; > > > The comment above this should probably be fixed. > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 5/5] virtio: Add bounce DMA ops
On Tue, 28 Apr 2020, Michael S. Tsirkin wrote: > On Tue, Apr 28, 2020 at 11:19:52PM +0530, Srivatsa Vaddagiri wrote: > > * Michael S. Tsirkin [2020-04-28 12:17:57]: > > > > > Okay, but how is all this virtio specific? For example, why not allow > > > separate swiotlbs for any type of device? > > > For example, this might make sense if a given device is from a > > > different, less trusted vendor. > > > > Is swiotlb commonly used for multiple devices that may be on different trust > > boundaries (and not behind a hardware iommu)? The trust boundary is not a good way of describing the scenario and I think it leads to miscommunication. A better way to describe the scenario would be that the device can only DMA to/from a small reserved-memory region advertised on device tree. Do we have other instances of devices that can only DMA to/from very specific and non-configurable address ranges? If so, this series could follow their example. > Even a hardware iommu does not imply a 100% security from malicious > hardware. First lots of people use iommu=pt for performance reasons. > Second even without pt, unmaps are often batched, and sub-page buffers > might be used for DMA, so we are not 100% protected at all times. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 5/5] virtio: Add bounce DMA ops
On Tue, 28 Apr 2020, Srivatsa Vaddagiri wrote:
> For better security, its desirable that a guest VM's memory is
> not accessible to any entity that executes outside the context of
> guest VM. In case of virtio, backend drivers execute outside the
> context of guest VM and in general will need access to complete
> guest VM memory. One option to restrict the access provided to
> backend driver is to make use of a bounce buffer. The bounce
> buffer is accessible to both backend and frontend drivers. All IO
> buffers that are in private space of guest VM are bounced to be
> accessible to backend.
[...]
> +static int __init virtio_bounce_setup(struct reserved_mem *rmem)
> +{
> + unsigned long node = rmem->fdt_node;
> +
> + if (!of_get_flat_dt_prop(node, "no-map", NULL))
> + return -EINVAL;
> +
> + return virtio_register_bounce_buffer(rmem->base, rmem->size);
> +}
> +
> +RESERVEDMEM_OF_DECLARE(virtio, "virtio_bounce_pool", virtio_bounce_setup);
Is this special reserved-memory region documented somewhere?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] xen/swiotlb: correct the check for xen_destroy_contiguous_region
On Tue, 28 Apr 2020, Jürgen Groß wrote: > On 28.04.20 09:33, peng@nxp.com wrote: > > From: Peng Fan > > > > When booting xen on i.MX8QM, met: > > " > > [3.602128] Unable to handle kernel paging request at virtual address > > 00272d40 > > [3.610804] Mem abort info: > > [3.613905] ESR = 0x9604 > > [3.617332] EC = 0x25: DABT (current EL), IL = 32 bits > > [3.623211] SET = 0, FnV = 0 > > [3.626628] EA = 0, S1PTW = 0 > > [3.630128] Data abort info: > > [3.633362] ISV = 0, ISS = 0x0004 > > [3.637630] CM = 0, WnR = 0 > > [3.640955] [00272d40] user address but active_mm is swapper > > [3.647983] Internal error: Oops: 9604 [#1] PREEMPT SMP > > [3.654137] Modules linked in: > > [3.677285] Hardware name: Freescale i.MX8QM MEK (DT) > > [3.677302] Workqueue: events deferred_probe_work_func > > [3.684253] imx6q-pcie 5f00.pcie: PCI host bridge to bus :00 > > [3.688297] pstate: 6005 (nZCv daif -PAN -UAO) > > [3.688310] pc : xen_swiotlb_free_coherent+0x180/0x1c0 > > [3.693993] pci_bus :00: root bus resource [bus 00-ff] > > [3.701002] lr : xen_swiotlb_free_coherent+0x44/0x1c0 > > " > > > > In xen_swiotlb_alloc_coherent, if !(dev_addr + size - 1 <= dma_mask) or > > range_straddles_page_boundary(phys, size) are true, it will > > create contiguous region. So when free, we need to free contiguous > > region use upper check condition. > > No, this will break PV guests on x86. > > I think there is something wrong with your setup in combination with > the ARM xen_create_contiguous_region() implementation. > > Stefano? Let me start by asking Peng a couple of questions: Peng, could you please add a printk to check which one of the two conditions is True for you? Is it (dev_addr + size - 1 > dma_mask) or range_straddles_page_boundary(phys, size)? Is hwdev->coherent_dma_mask set for your DMA capable device? Finally, is this patch supposed to fix the crash you are seeing? If not, can you tell where is the crash exactly? Juergen, keep in mind that xen_create_contiguous_region does nothing on ARM because in dom0 guest_phys == phys. xen_create_contiguous_region simply sets dma_handle to phys. Whatever condition caused the code to take the xen_create_contiguous_region branch in xen_swiotlb_alloc_coherent, it will also cause it to WARN in xen_swiotlb_free_coherent. range_straddles_page_boundary should never return True because guest_phys == phys. That leaves us with the dma_mask check: dev_addr + size - 1 <= dma_mask dev_addr is the dma_handle allocated by xen_alloc_coherent_pages. dma_mask is either DMA_BIT_MASK(32) or hwdev->coherent_dma_mask. The implementation of xen_alloc_coherent_pages has been recently changed to use dma_direct_alloc. Christoff, does dma_direct_alloc respect hwdev->coherent_dma_mask if present? Also, can it return highmem pages? > Juergen > > > > > Signed-off-by: Peng Fan > > --- > > drivers/xen/swiotlb-xen.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > > index b6d27762c6f8..ab96e468584f 100644 > > --- a/drivers/xen/swiotlb-xen.c > > +++ b/drivers/xen/swiotlb-xen.c > > @@ -346,8 +346,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t > > size, void *vaddr, > > /* Convert the size to actually allocated. */ > > size = 1UL << (order + XEN_PAGE_SHIFT); > > - if (!WARN_ON((dev_addr + size - 1 > dma_mask) || > > -range_straddles_page_boundary(phys, size)) && > > + if (((dev_addr + size - 1 > dma_mask) || > > + range_straddles_page_boundary(phys, size)) && > > TestClearPageXenRemapped(virt_to_page(vaddr))) > > xen_destroy_contiguous_region(phys, order); > > > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 01/11] xen/arm: use dma-noncoherent.h calls for xen-swiotlb cache maintainance
On Thu, 5 Sep 2019, Christoph Hellwig wrote:
> Copy the arm64 code that uses the dma-direct/swiotlb helpers for DMA
> on-coherent devices.
>
> Signed-off-by: Christoph Hellwig
This is much better and much more readable.
Reviewed-by: Stefano Stabellini
> ---
> arch/arm/include/asm/device.h| 3 -
> arch/arm/include/asm/xen/page-coherent.h | 72 +---
> arch/arm/mm/dma-mapping.c| 8 +--
> drivers/xen/swiotlb-xen.c| 20 ---
> 4 files changed, 28 insertions(+), 75 deletions(-)
>
> diff --git a/arch/arm/include/asm/device.h b/arch/arm/include/asm/device.h
> index f6955b55c544..c675bc0d5aa8 100644
> --- a/arch/arm/include/asm/device.h
> +++ b/arch/arm/include/asm/device.h
> @@ -14,9 +14,6 @@ struct dev_archdata {
> #endif
> #ifdef CONFIG_ARM_DMA_USE_IOMMU
> struct dma_iommu_mapping*mapping;
> -#endif
> -#ifdef CONFIG_XEN
> - const struct dma_map_ops *dev_dma_ops;
> #endif
> unsigned int dma_coherent:1;
> unsigned int dma_ops_setup:1;
> diff --git a/arch/arm/include/asm/xen/page-coherent.h
> b/arch/arm/include/asm/xen/page-coherent.h
> index 2c403e7c782d..602ac02f154c 100644
> --- a/arch/arm/include/asm/xen/page-coherent.h
> +++ b/arch/arm/include/asm/xen/page-coherent.h
> @@ -6,23 +6,37 @@
> #include
> #include
>
> -static inline const struct dma_map_ops *xen_get_dma_ops(struct device *dev)
> -{
> - if (dev && dev->archdata.dev_dma_ops)
> - return dev->archdata.dev_dma_ops;
> - return get_arch_dma_ops(NULL);
> -}
> -
> static inline void *xen_alloc_coherent_pages(struct device *hwdev, size_t
> size,
> dma_addr_t *dma_handle, gfp_t flags, unsigned long attrs)
> {
> - return xen_get_dma_ops(hwdev)->alloc(hwdev, size, dma_handle, flags,
> attrs);
> + return dma_direct_alloc(hwdev, size, dma_handle, flags, attrs);
> }
>
> static inline void xen_free_coherent_pages(struct device *hwdev, size_t size,
> void *cpu_addr, dma_addr_t dma_handle, unsigned long attrs)
> {
> - xen_get_dma_ops(hwdev)->free(hwdev, size, cpu_addr, dma_handle, attrs);
> + dma_direct_free(hwdev, size, cpu_addr, dma_handle, 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)
> +{
> + 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,
> @@ -36,17 +50,8 @@ static inline void xen_dma_map_page(struct device *hwdev,
> struct page *page,
> bool local = (page_pfn <= dev_pfn) &&
> (dev_pfn - page_pfn < compound_pages);
>
> - /*
> - * Dom0 is mapped 1:1, while the Linux page can span across
> - * multiple Xen pages, it's not possible for it to contain a
> - * mix of local and foreign Xen pages. So if the first xen_pfn
> - * == mfn the page is local otherwise it's a foreign page
> - * grant-mapped in dom0. If the page is local we can safely
> - * call the native dma_ops function, otherwise we call the xen
> - * specific function.
> - */
> if (local)
> - xen_get_dma_ops(hwdev)->map_page(hwdev, page, offset, size,
> dir, attrs);
> + dma_direct_map_page(hwdev, page, offset, size, dir, attrs);
> else
> __xen_dma_map_page(hwdev, page, dev_addr, offset, size, dir,
> attrs);
> }
> @@ -63,33 +68,10 @@ static inline void xen_dma_unmap_page(struct device
> *hwdev, dma_addr_t handle,
>* safely call the native dma_ops function, otherwise we call the xen
>* specific function.
>*/
> - if (pfn_valid(pfn)) {
> - if (xen_get_dma_ops(hwdev)->unmap_page)
> - xen_get_dma_ops(hwdev)->unmap_page(hwdev, handle, size,
> dir, attrs);
> - } else
> + if (pfn_valid(pfn))
> + dma_direct_unmap_page(hwdev, handle, size, dir, attrs);
>
Re: [PATCH 02/11] xen/arm: consolidate page-coherent.h
On Thu, 5 Sep 2019, Christoph Hellwig wrote:
> Shared the duplicate arm/arm64 code in include/xen/arm/page-coherent.h.
>
> Signed-off-by: Christoph Hellwig
Reviewed-by: Stefano Stabellini
> ---
> arch/arm/include/asm/xen/page-coherent.h | 75
> arch/arm64/include/asm/xen/page-coherent.h | 75
> include/xen/arm/page-coherent.h| 80 ++
> 3 files changed, 80 insertions(+), 150 deletions(-)
>
> diff --git a/arch/arm/include/asm/xen/page-coherent.h
> b/arch/arm/include/asm/xen/page-coherent.h
> index 602ac02f154c..27e984977402 100644
> --- a/arch/arm/include/asm/xen/page-coherent.h
> +++ b/arch/arm/include/asm/xen/page-coherent.h
> @@ -1,77 +1,2 @@
> /* SPDX-License-Identifier: GPL-2.0 */
> -#ifndef _ASM_ARM_XEN_PAGE_COHERENT_H
> -#define _ASM_ARM_XEN_PAGE_COHERENT_H
> -
> -#include
> -#include
> #include
> -
> -static inline void *xen_alloc_coherent_pages(struct device *hwdev, size_t
> size,
> - dma_addr_t *dma_handle, gfp_t flags, unsigned long attrs)
> -{
> - return dma_direct_alloc(hwdev, size, dma_handle, flags, attrs);
> -}
> -
> -static inline void xen_free_coherent_pages(struct device *hwdev, size_t size,
> - void *cpu_addr, dma_addr_t dma_handle, unsigned long attrs)
> -{
> - dma_direct_free(hwdev, size, cpu_addr, dma_handle, 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)
> -{
> - 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 page_pfn = page_to_xen_pfn(page);
> - unsigned long dev_pfn = XEN_PFN_DOWN(dev_addr);
> - unsigned long compound_pages =
> - (1< - bool local = (page_pfn <= dev_pfn) &&
> - (dev_pfn - page_pfn < compound_pages);
> -
> - if (local)
> - 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);
> - /*
> - * Dom0 is mapped 1:1, while the Linux page can be spanned accross
> - * multiple Xen page, it's not possible to have a mix of local and
> - * foreign Xen page. Dom0 is mapped 1:1, so calling pfn_valid on a
> - * foreign mfn will always return false. If the page is local we can
> - * safely call the native dma_ops function, otherwise we call the xen
> - * specific function.
> - */
> - if (pfn_valid(pfn))
> - dma_direct_unmap_page(hwdev, handle, size, dir, attrs);
> - else
> - __xen_dma_unmap_page(hwdev, handle, size, dir, attrs);
> -}
> -
> -#endif /* _ASM_ARM_XEN_PAGE_COHERENT_H */
> diff --git a/arch/arm64/include/asm/xen/page-coherent.h
> b/arch/arm64/include/asm/xen/page-coherent.h
> index d88e56b90b93..27e984977402 100644
> --- a/arch/arm64/include/asm/xen/page-coherent.h
> +++ b/arch/arm64/include/asm/xen/page-coherent.h
> @@ -1,77 +1,2 @@
> /* SPDX-License-Identifier: GPL-2.0 */
> -#ifndef _ASM_ARM64_XEN_PAGE_COHERENT_H
> -#define _ASM_ARM64_XEN_PAGE_COHERENT_H
> -
> -#include
> -#include
> #include
> -
> -static inline void *xen_alloc_coherent_pages(struct device *hwdev, size_t
> size,
> - dma_addr_t *dma_handle, gfp_t flags, unsigned long attrs)
> -{
> - return dma_direct_alloc(hwdev, size, dma_handle, flags, attrs);
> -}
> -
> -static inline void xen_free_coherent_pages(struct device *hwdev, size_t size,
> - void *cpu_addr, dma_addr_t dma_handle, unsigned long attrs)
>
Re: swiotlb-xen cleanups v2
On Mon, 26 Aug 2019, Christoph Hellwig wrote: > Hi Xen maintainers and friends, > > please take a look at this series that cleans up the parts of swiotlb-xen > that deal with non-coherent caches. > > Changes since v1: > - rewrite dma_cache_maint to be much simpler > - improve various comments and commit logs > - remove page-coherent.h entirely Thanks for your work on this, it really makes the code better. I tested it on ARM64 with a non-coherent network device and verified it works as intended (Cadence GEM on ZynqMP).
Re: [PATCH 11/11] arm64: use asm-generic/dma-mapping.h
On Mon, 26 Aug 2019, Christoph Hellwig wrote:
> Now that the Xen special cases are gone nothing worth mentioning is
> left in the arm64 file, so switch to use the
> asm-generic version instead.
>
> Signed-off-by: Christoph Hellwig
> Acked-by: Will Deacon
Reviewed-by: Stefano Stabellini
> ---
> arch/arm64/include/asm/Kbuild| 1 +
> arch/arm64/include/asm/dma-mapping.h | 22 --
> arch/arm64/mm/dma-mapping.c | 1 +
> 3 files changed, 2 insertions(+), 22 deletions(-)
> delete mode 100644 arch/arm64/include/asm/dma-mapping.h
>
> diff --git a/arch/arm64/include/asm/Kbuild b/arch/arm64/include/asm/Kbuild
> index c52e151afab0..98a5405c8558 100644
> --- a/arch/arm64/include/asm/Kbuild
> +++ b/arch/arm64/include/asm/Kbuild
> @@ -4,6 +4,7 @@ generic-y += delay.h
> generic-y += div64.h
> generic-y += dma.h
> generic-y += dma-contiguous.h
> +generic-y += dma-mapping.h
> generic-y += early_ioremap.h
> generic-y += emergency-restart.h
> generic-y += hw_irq.h
> diff --git a/arch/arm64/include/asm/dma-mapping.h
> b/arch/arm64/include/asm/dma-mapping.h
> deleted file mode 100644
> index 67243255a858..
> --- a/arch/arm64/include/asm/dma-mapping.h
> +++ /dev/null
> @@ -1,22 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-only */
> -/*
> - * Copyright (C) 2012 ARM Ltd.
> - */
> -#ifndef __ASM_DMA_MAPPING_H
> -#define __ASM_DMA_MAPPING_H
> -
> -#ifdef __KERNEL__
> -
> -#include
> -#include
> -
> -#include
> -#include
> -
> -static inline const struct dma_map_ops *get_arch_dma_ops(struct bus_type
> *bus)
> -{
> - return NULL;
> -}
> -
> -#endif /* __KERNEL__ */
> -#endif /* __ASM_DMA_MAPPING_H */
> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> index 4b244a037349..6578abcfbbc7 100644
> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch/arm64/mm/dma-mapping.c
> @@ -8,6 +8,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include
> --
> 2.20.1
>
Re: [PATCH 03/11] xen/arm: simplify dma_cache_maint
On Tue, 27 Aug 2019, Christoph Hellwig wrote:
> And this was still buggy I think, it really needs some real Xen/Arm
> testing which I can't do. Hopefully better version below:
>
> --
> >From 5ad4b6e291dbb49f65480c9b769414931cbd485a Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig
> Date: Wed, 24 Jul 2019 15:26:08 +0200
> Subject: xen/arm: simplify dma_cache_maint
>
> Calculate the required operation in the caller, and pass it directly
> instead of recalculating it for each page, and use simple arithmetics
> to get from the physical address to Xen page size aligned chunks.
>
> Signed-off-by: Christoph Hellwig
> ---
> arch/arm/xen/mm.c | 61 ---
> 1 file changed, 21 insertions(+), 40 deletions(-)
>
> diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
> index 90574d89d0d4..2fde161733b0 100644
> --- a/arch/arm/xen/mm.c
> +++ b/arch/arm/xen/mm.c
> @@ -35,64 +35,45 @@ unsigned long xen_get_swiotlb_free_pages(unsigned int
> order)
> return __get_free_pages(flags, order);
> }
>
> -enum dma_cache_op {
> - DMA_UNMAP,
> - DMA_MAP,
> -};
> static bool hypercall_cflush = false;
>
> -/* functions called by SWIOTLB */
> -
> -static void dma_cache_maint(dma_addr_t handle, unsigned long offset,
> - size_t size, enum dma_data_direction dir, enum dma_cache_op op)
> +/* buffers in highmem or foreign pages cannot cross page boundaries */
> +static void dma_cache_maint(dma_addr_t handle, size_t size, u32 op)
> {
> struct gnttab_cache_flush cflush;
> - unsigned long xen_pfn;
> - size_t left = size;
>
> - xen_pfn = (handle >> XEN_PAGE_SHIFT) + offset / XEN_PAGE_SIZE;
> - offset %= XEN_PAGE_SIZE;
> + cflush.a.dev_bus_addr = handle & XEN_PAGE_MASK;
> + cflush.offset = xen_offset_in_page(handle);
> + cflush.op = op;
>
> do {
> - size_t len = left;
> -
> - /* buffers in highmem or foreign pages cannot cross page
> - * boundaries */
> - if (len + offset > XEN_PAGE_SIZE)
> - len = XEN_PAGE_SIZE - offset;
> -
> - cflush.op = 0;
> - cflush.a.dev_bus_addr = xen_pfn << XEN_PAGE_SHIFT;
> - cflush.offset = offset;
> - cflush.length = len;
> -
> - if (op == DMA_UNMAP && dir != DMA_TO_DEVICE)
> - cflush.op = GNTTAB_CACHE_INVAL;
> - if (op == DMA_MAP) {
> - if (dir == DMA_FROM_DEVICE)
> - cflush.op = GNTTAB_CACHE_INVAL;
> - else
> - cflush.op = GNTTAB_CACHE_CLEAN;
> - }
> - if (cflush.op)
> - HYPERVISOR_grant_table_op(GNTTABOP_cache_flush,
> , 1);
> + if (size + cflush.offset > XEN_PAGE_SIZE)
> + cflush.length = XEN_PAGE_SIZE - cflush.offset;
> + else
> + cflush.length = size;
> +
> + HYPERVISOR_grant_table_op(GNTTABOP_cache_flush, , 1);
>
> - offset = 0;
> - xen_pfn++;
> - left -= len;
> - } while (left);
> + cflush.offset = 0;
> + cflush.a.dev_bus_addr += cflush.length;
> + size -= cflush.length;
Yes that's better
Reviewed-by: Stefano Stabellini
> + } 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)
> {
> - dma_cache_maint(handle & PAGE_MASK, handle & ~PAGE_MASK, size, dir,
> DMA_UNMAP);
> + 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)
> {
> - dma_cache_maint(handle & PAGE_MASK, handle & ~PAGE_MASK, size, dir,
> DMA_MAP);
> + 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,
> --
> 2.20.1
>
Re: [PATCH 09/11] swiotlb-xen: remove page-coherent.h
On Mon, 26 Aug 2019, Christoph Hellwig wrote:
> The only thing left of page-coherent.h is two functions implemented by
> the architecture for non-coherent DMA support that are never called for
> fully coherent architectures. Just move the prototypes for those to
> swiotlb-xen.h instead.
>
> Signed-off-by: Christoph Hellwig
Reviewed-by: Stefano Stabellini
> ---
> arch/arm/include/asm/xen/page-coherent.h | 2 --
> arch/arm64/include/asm/xen/page-coherent.h | 2 --
> arch/x86/include/asm/xen/page-coherent.h | 11 ---
> drivers/xen/swiotlb-xen.c | 3 ---
> include/Kbuild | 1 -
> include/xen/arm/page-coherent.h| 10 --
> include/xen/swiotlb-xen.h | 6 ++
> 7 files changed, 6 insertions(+), 29 deletions(-)
> delete mode 100644 arch/arm/include/asm/xen/page-coherent.h
> delete mode 100644 arch/arm64/include/asm/xen/page-coherent.h
> delete mode 100644 arch/x86/include/asm/xen/page-coherent.h
> delete mode 100644 include/xen/arm/page-coherent.h
>
> diff --git a/arch/arm/include/asm/xen/page-coherent.h
> b/arch/arm/include/asm/xen/page-coherent.h
> deleted file mode 100644
> index 27e984977402..
> --- a/arch/arm/include/asm/xen/page-coherent.h
> +++ /dev/null
> @@ -1,2 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> -#include
> diff --git a/arch/arm64/include/asm/xen/page-coherent.h
> b/arch/arm64/include/asm/xen/page-coherent.h
> deleted file mode 100644
> index 27e984977402..
> --- a/arch/arm64/include/asm/xen/page-coherent.h
> +++ /dev/null
> @@ -1,2 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> -#include
> diff --git a/arch/x86/include/asm/xen/page-coherent.h
> b/arch/x86/include/asm/xen/page-coherent.h
> deleted file mode 100644
> index c9c8398a31ff..
> --- a/arch/x86/include/asm/xen/page-coherent.h
> +++ /dev/null
> @@ -1,11 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> -#ifndef _ASM_X86_XEN_PAGE_COHERENT_H
> -#define _ASM_X86_XEN_PAGE_COHERENT_H
> -
> -static inline void xen_dma_sync_single_for_cpu(struct device *hwdev,
> - dma_addr_t handle, size_t size, enum dma_data_direction 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) { }
> -
> -#endif /* _ASM_X86_XEN_PAGE_COHERENT_H */
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index a642e284f1e2..95911ff9c11c 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -35,9 +35,6 @@
> #include
> #include
>
> -#include
> -#include
> -
> #include
> /*
> * Used to do a quick range check in swiotlb_tbl_unmap_single and
> diff --git a/include/Kbuild b/include/Kbuild
> index c38f0d46b267..cce5cf6abf89 100644
> --- a/include/Kbuild
> +++ b/include/Kbuild
> @@ -1189,7 +1189,6 @@ header-test-+= video/vga.h
> header-test- += video/w100fb.h
> header-test- += xen/acpi.h
> header-test- += xen/arm/hypercall.h
> -header-test- += xen/arm/page-coherent.h
> header-test- += xen/arm/page.h
> header-test- += xen/balloon.h
> header-test- += xen/events.h
> diff --git a/include/xen/arm/page-coherent.h b/include/xen/arm/page-coherent.h
> deleted file mode 100644
> index 635492d41ebe..
> --- a/include/xen/arm/page-coherent.h
> +++ /dev/null
> @@ -1,10 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> -#ifndef _XEN_ARM_PAGE_COHERENT_H
> -#define _XEN_ARM_PAGE_COHERENT_H
> -
> -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 */
> diff --git a/include/xen/swiotlb-xen.h b/include/xen/swiotlb-xen.h
> index 5e4b83f83dbc..a7c642872568 100644
> --- a/include/xen/swiotlb-xen.h
> +++ b/include/xen/swiotlb-xen.h
> @@ -2,8 +2,14 @@
> #ifndef __LINUX_SWIOTLB_XEN_H
> #define __LINUX_SWIOTLB_XEN_H
>
> +#include
> #include
>
> +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);
> +
> extern int xen_swiotlb_init(int verbose, bool early);
> extern const struct dma_map_ops xen_swiotlb_dma_ops;
>
> --
> 2.20.1
>
Re: [PATCH 10/11] swiotlb-xen: merge xen_unmap_single into xen_swiotlb_unmap_page
On Mon, 26 Aug 2019, Christoph Hellwig wrote:
> No need for a no-op wrapper.
>
> Signed-off-by: Christoph Hellwig
Reviewed-by: Stefano Stabellini
> ---
> drivers/xen/swiotlb-xen.c | 15 ---
> 1 file changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index 95911ff9c11c..384304a77020 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -414,9 +414,8 @@ static dma_addr_t xen_swiotlb_map_page(struct device
> *dev, struct page *page,
> * After this call, reads by the cpu to the buffer are guaranteed to see
> * whatever the device wrote there.
> */
> -static void xen_unmap_single(struct device *hwdev, dma_addr_t dev_addr,
> - size_t size, enum dma_data_direction dir,
> - unsigned long attrs)
> +static void xen_swiotlb_unmap_page(struct device *hwdev, dma_addr_t dev_addr,
> + size_t size, enum dma_data_direction dir, unsigned long attrs)
> {
> phys_addr_t paddr = xen_bus_to_phys(dev_addr);
>
> @@ -430,13 +429,6 @@ static void xen_unmap_single(struct device *hwdev,
> dma_addr_t dev_addr,
> swiotlb_tbl_unmap_single(hwdev, paddr, size, dir, attrs);
> }
>
> -static void xen_swiotlb_unmap_page(struct device *hwdev, dma_addr_t dev_addr,
> - size_t size, enum dma_data_direction dir,
> - unsigned long attrs)
> -{
> - xen_unmap_single(hwdev, dev_addr, size, dir, attrs);
> -}
> -
> static void
> xen_swiotlb_sync_single_for_cpu(struct device *dev, dma_addr_t dma_addr,
> size_t size, enum dma_data_direction dir)
> @@ -477,7 +469,8 @@ xen_swiotlb_unmap_sg(struct device *hwdev, struct
> scatterlist *sgl, int nelems,
> BUG_ON(dir == DMA_NONE);
>
> for_each_sg(sgl, sg, nelems, i)
> - xen_unmap_single(hwdev, sg->dma_address, sg_dma_len(sg), dir,
> attrs);
> + xen_swiotlb_unmap_page(hwdev, sg->dma_address, sg_dma_len(sg),
> + dir, attrs);
>
> }
>
> --
> 2.20.1
>
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
>
> ---
> 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
> 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-c
Re: [PATCH 06/11] swiotlb-xen: always use dma-direct helpers to alloc coherent pages
+ Boris, Juergen
On Mon, 26 Aug 2019, Christoph Hellwig wrote:
> x86 currently calls alloc_pages, but using dma-direct works as well
> there, with the added benefit of using the CMA pool if available.
> The biggest advantage is of course to remove a pointless bit of
> architecture specific code.
>
> Signed-off-by: Christoph Hellwig
> ---
> arch/x86/include/asm/xen/page-coherent.h | 16
> drivers/xen/swiotlb-xen.c| 7 +++
> include/xen/arm/page-coherent.h | 12
> 3 files changed, 3 insertions(+), 32 deletions(-)
>
> diff --git a/arch/x86/include/asm/xen/page-coherent.h
> b/arch/x86/include/asm/xen/page-coherent.h
> index 116777e7f387..8ee33c5edded 100644
> --- a/arch/x86/include/asm/xen/page-coherent.h
> +++ b/arch/x86/include/asm/xen/page-coherent.h
> @@ -5,22 +5,6 @@
> #include
> #include
>
> -static inline void *xen_alloc_coherent_pages(struct device *hwdev, size_t
> size,
> - dma_addr_t *dma_handle, gfp_t flags,
> - unsigned long attrs)
> -{
> - void *vstart = (void*)__get_free_pages(flags, get_order(size));
> - *dma_handle = virt_to_phys(vstart);
This is where we need Boris and Juergen's opinion. From an ARM POV it
looks OK.
> - return vstart;
> -}
> -
> -static inline void xen_free_coherent_pages(struct device *hwdev, size_t size,
> - void *cpu_addr, dma_addr_t dma_handle,
> - unsigned long attrs)
> -{
> - free_pages((unsigned long) cpu_addr, get_order(size));
> -}
> -
> 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) { }
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index b8808677ae1d..f9dd4cb6e4b3 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -299,8 +299,7 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t
> size,
>* address. In fact on ARM virt_to_phys only works for kernel direct
>* mapped RAM memory. Also see comment below.
>*/
> - ret = xen_alloc_coherent_pages(hwdev, size, dma_handle, flags, attrs);
> -
> + ret = dma_direct_alloc(hwdev, size, dma_handle, flags, attrs);
> if (!ret)
> return ret;
>
> @@ -319,7 +318,7 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t
> size,
> else {
> if (xen_create_contiguous_region(phys, order,
>fls64(dma_mask), dma_handle)
> != 0) {
> - xen_free_coherent_pages(hwdev, size, ret,
> (dma_addr_t)phys, attrs);
> + dma_direct_free(hwdev, size, ret, (dma_addr_t)phys,
> attrs);
> return NULL;
> }
> SetPageXenRemapped(virt_to_page(ret));
> @@ -351,7 +350,7 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t
> size, void *vaddr,
> TestClearPageXenRemapped(virt_to_page(vaddr)))
> xen_destroy_contiguous_region(phys, order);
>
> - xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs);
> + dma_direct_free(hwdev, size, vaddr, (dma_addr_t)phys, attrs);
> }
>
> /*
> diff --git a/include/xen/arm/page-coherent.h b/include/xen/arm/page-coherent.h
> index a840d6949a87..0e244f4fec1a 100644
> --- a/include/xen/arm/page-coherent.h
> +++ b/include/xen/arm/page-coherent.h
> @@ -16,18 +16,6 @@ void __xen_dma_sync_single_for_cpu(struct device *hwdev,
> 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_alloc_coherent_pages(struct device *hwdev, size_t
> size,
> - dma_addr_t *dma_handle, gfp_t flags, unsigned long attrs)
> -{
> - return dma_direct_alloc(hwdev, size, dma_handle, flags, attrs);
> -}
> -
> -static inline void xen_free_coherent_pages(struct device *hwdev, size_t size,
> - void *cpu_addr, dma_addr_t dma_handle, unsigned long attrs)
> -{
> - dma_direct_free(hwdev, size, cpu_addr, dma_handle, 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)
> {
> --
> 2.20.1
>
Re: [PATCH 07/11] swiotlb-xen: use the same foreign page check everywhere
On Mon, 26 Aug 2019, Christoph Hellwig wrote:
> xen_dma_map_page uses a different and more complicated check for foreign
> pages than the other three cache maintainance helpers. Switch it to the
> simpler pfn_valid method a well, and document the scheme with a single
> improved comment in xen_dma_map_page.
>
> Signed-off-by: Christoph Hellwig
Reviewed-by: Stefano Stabellini
> ---
> include/xen/arm/page-coherent.h | 31 +--
> 1 file changed, 9 insertions(+), 22 deletions(-)
>
> diff --git a/include/xen/arm/page-coherent.h b/include/xen/arm/page-coherent.h
> index 0e244f4fec1a..07c104dbc21f 100644
> --- a/include/xen/arm/page-coherent.h
> +++ b/include/xen/arm/page-coherent.h
> @@ -41,23 +41,17 @@ 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 page_pfn = page_to_xen_pfn(page);
> - unsigned long dev_pfn = XEN_PFN_DOWN(dev_addr);
> - unsigned long compound_pages =
> - (1< - bool local = (page_pfn <= dev_pfn) &&
> - (dev_pfn - page_pfn < compound_pages);
> + unsigned long pfn = PFN_DOWN(dev_addr);
>
> /*
> - * Dom0 is mapped 1:1, while the Linux page can span across
> - * multiple Xen pages, it's not possible for it to contain a
> - * mix of local and foreign Xen pages. So if the first xen_pfn
> - * == mfn the page is local otherwise it's a foreign page
> - * grant-mapped in dom0. If the page is local we can safely
> - * call the native dma_ops function, otherwise we call the xen
> - * specific function.
> + * 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 (local)
> + 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);
> @@ -67,14 +61,7 @@ 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);
> - /*
> - * Dom0 is mapped 1:1, while the Linux page can be spanned accross
> - * multiple Xen page, it's not possible to have a mix of local and
> - * foreign Xen page. Dom0 is mapped 1:1, so calling pfn_valid on a
> - * foreign mfn will always return false. If the page is local we can
> - * safely call the native dma_ops function, otherwise we call the xen
> - * specific function.
> - */
> +
> if (pfn_valid(pfn))
> dma_direct_unmap_page(hwdev, handle, size, dir, attrs);
> else
> --
> 2.20.1
>
Re: [PATCH 03/11] xen/arm: simplify dma_cache_maint
On Mon, 26 Aug 2019, Christoph Hellwig wrote:
> Calculate the required operation in the caller, and pass it directly
> instead of recalculating it for each page, and use simple arithmetics
> to get from the physical address to Xen page size aligned chunks.
>
> Signed-off-by: Christoph Hellwig
> ---
> arch/arm/xen/mm.c | 62 +--
> 1 file changed, 22 insertions(+), 40 deletions(-)
>
> diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
> index 90574d89d0d4..14210ebdea1a 100644
> --- a/arch/arm/xen/mm.c
> +++ b/arch/arm/xen/mm.c
> @@ -35,64 +35,46 @@ unsigned long xen_get_swiotlb_free_pages(unsigned int
> order)
> return __get_free_pages(flags, order);
> }
>
> -enum dma_cache_op {
> - DMA_UNMAP,
> - DMA_MAP,
> -};
> static bool hypercall_cflush = false;
>
> -/* functions called by SWIOTLB */
> -
> -static void dma_cache_maint(dma_addr_t handle, unsigned long offset,
> - size_t size, enum dma_data_direction dir, enum dma_cache_op op)
> +/* buffers in highmem or foreign pages cannot cross page boundaries */
> +static void dma_cache_maint(dma_addr_t handle, size_t size, u32 op)
> {
> struct gnttab_cache_flush cflush;
> - unsigned long xen_pfn;
> - size_t left = size;
>
> - xen_pfn = (handle >> XEN_PAGE_SHIFT) + offset / XEN_PAGE_SIZE;
> - offset %= XEN_PAGE_SIZE;
> + cflush.a.dev_bus_addr = handle & XEN_PAGE_MASK;
> + cflush.offset = xen_offset_in_page(handle);
> + cflush.op = op;
>
> do {
> - size_t len = left;
> -
> - /* buffers in highmem or foreign pages cannot cross page
> - * boundaries */
> - if (len + offset > XEN_PAGE_SIZE)
> - len = XEN_PAGE_SIZE - offset;
> -
> - cflush.op = 0;
> - cflush.a.dev_bus_addr = xen_pfn << XEN_PAGE_SHIFT;
> - cflush.offset = offset;
> - cflush.length = len;
> -
> - if (op == DMA_UNMAP && dir != DMA_TO_DEVICE)
> - cflush.op = GNTTAB_CACHE_INVAL;
> - if (op == DMA_MAP) {
> - if (dir == DMA_FROM_DEVICE)
> - cflush.op = GNTTAB_CACHE_INVAL;
> - else
> - cflush.op = GNTTAB_CACHE_CLEAN;
> - }
> - if (cflush.op)
> - HYPERVISOR_grant_table_op(GNTTABOP_cache_flush,
> , 1);
> + if (size + cflush.offset > XEN_PAGE_SIZE)
> + cflush.length = XEN_PAGE_SIZE - cflush.offset;
> + else
> + cflush.length = size;
isn't it missing a:
cflush.a.dev_bus_addr = handle & XEN_PAGE_MASK;
here?
> + HYPERVISOR_grant_table_op(GNTTABOP_cache_flush, , 1);
> +
> + handle += cflush.length;
> + size -= cflush.length;
>
> - offset = 0;
> - xen_pfn++;
> - left -= len;
> - } while (left);
> + cflush.offset = 0;
> + } 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)
> {
> - dma_cache_maint(handle & PAGE_MASK, handle & ~PAGE_MASK, size, dir,
> DMA_UNMAP);
> + 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)
> {
> - dma_cache_maint(handle & PAGE_MASK, handle & ~PAGE_MASK, size, dir,
> DMA_MAP);
> + 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,
> --
> 2.20.1
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 02/11] xen/arm: use dev_is_dma_coherent
On Mon, 26 Aug 2019, Christoph Hellwig wrote:
> Use the dma-noncoherent dev_is_dma_coherent helper instead of the home
> grown variant. Note that both are always initialized to the same
> value in arch_setup_dma_ops.
>
> Signed-off-by: Christoph Hellwig
> Reviewed-by: Julien Grall
Reviewed-by: Stefano Stabellini
> ---
> arch/arm/include/asm/dma-mapping.h | 6 --
> arch/arm/xen/mm.c| 12 ++--
> arch/arm64/include/asm/dma-mapping.h | 9 -
> 3 files changed, 6 insertions(+), 21 deletions(-)
>
> diff --git a/arch/arm/include/asm/dma-mapping.h
> b/arch/arm/include/asm/dma-mapping.h
> index dba9355e2484..bdd80ddbca34 100644
> --- a/arch/arm/include/asm/dma-mapping.h
> +++ b/arch/arm/include/asm/dma-mapping.h
> @@ -91,12 +91,6 @@ static inline dma_addr_t virt_to_dma(struct device *dev,
> void *addr)
> }
> #endif
>
> -/* do not use this function in a driver */
> -static inline bool is_device_dma_coherent(struct device *dev)
> -{
> - return dev->archdata.dma_coherent;
> -}
> -
> /**
> * arm_dma_alloc - allocate consistent memory for DMA
> * @dev: valid struct device pointer, or NULL for ISA and EISA-like devices
> diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
> index d33b77e9add3..90574d89d0d4 100644
> --- a/arch/arm/xen/mm.c
> +++ b/arch/arm/xen/mm.c
> @@ -1,6 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0-only
> #include
> -#include
> +#include
> #include
> #include
> #include
> @@ -99,7 +99,7 @@ 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 (is_device_dma_coherent(hwdev))
> + if (dev_is_dma_coherent(hwdev))
> return;
> if (attrs & DMA_ATTR_SKIP_CPU_SYNC)
> return;
> @@ -112,7 +112,7 @@ void __xen_dma_unmap_page(struct device *hwdev,
> dma_addr_t handle,
> unsigned long attrs)
>
> {
> - if (is_device_dma_coherent(hwdev))
> + if (dev_is_dma_coherent(hwdev))
> return;
> if (attrs & DMA_ATTR_SKIP_CPU_SYNC)
> return;
> @@ -123,7 +123,7 @@ void __xen_dma_unmap_page(struct device *hwdev,
> dma_addr_t handle,
> void __xen_dma_sync_single_for_cpu(struct device *hwdev,
> dma_addr_t handle, size_t size, enum dma_data_direction dir)
> {
> - if (is_device_dma_coherent(hwdev))
> + if (dev_is_dma_coherent(hwdev))
> return;
> __xen_dma_page_dev_to_cpu(hwdev, handle, size, dir);
> }
> @@ -131,7 +131,7 @@ void __xen_dma_sync_single_for_cpu(struct device *hwdev,
> void __xen_dma_sync_single_for_device(struct device *hwdev,
> dma_addr_t handle, size_t size, enum dma_data_direction dir)
> {
> - if (is_device_dma_coherent(hwdev))
> + if (dev_is_dma_coherent(hwdev))
> return;
> __xen_dma_page_cpu_to_dev(hwdev, handle, size, dir);
> }
> @@ -159,7 +159,7 @@ bool xen_arch_need_swiotlb(struct device *dev,
>* memory and we are not able to flush the cache.
>*/
> return (!hypercall_cflush && (xen_pfn != bfn) &&
> - !is_device_dma_coherent(dev));
> + !dev_is_dma_coherent(dev));
> }
>
> int xen_create_contiguous_region(phys_addr_t pstart, unsigned int order,
> diff --git a/arch/arm64/include/asm/dma-mapping.h
> b/arch/arm64/include/asm/dma-mapping.h
> index bdcb0922a40c..67243255a858 100644
> --- a/arch/arm64/include/asm/dma-mapping.h
> +++ b/arch/arm64/include/asm/dma-mapping.h
> @@ -18,14 +18,5 @@ static inline const struct dma_map_ops
> *get_arch_dma_ops(struct bus_type *bus)
> return NULL;
> }
>
> -/*
> - * Do not use this function in a driver, it is only provided for
> - * arch/arm/mm/xen.c, which is used by arm64 as well.
> - */
> -static inline bool is_device_dma_coherent(struct device *dev)
> -{
> - return dev->dma_coherent;
> -}
> -
> #endif /* __KERNEL__ */
> #endif /* __ASM_DMA_MAPPING_H */
> --
> 2.20.1
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 01/11] xen/arm: use dma-noncoherent.h calls for xen-swiotlb cache maintainance
On Mon, 26 Aug 2019, Christoph Hellwig wrote:
> Reuse the arm64 code that uses the dma-direct/swiotlb helpers for DMA
> non-coherent devices.
This patch does a bunch of things not listed in the commit message, such
as moving the static inline functions to include/xen/arm/page-coherent.h
and removing xen_swiotlb_dma_mmap and xen_swiotlb_get_sgtable because
unnecessary.
I would prefer if they were separate patches (for bisectability). It's
OK if you want to keep it all in one patch but please list all changes
the commit message.
In any case, I looked at the patch in details and it does all the right
things -- it's correct.
> Signed-off-by: Christoph Hellwig
> ---
> arch/arm/include/asm/device.h | 3 -
> arch/arm/include/asm/xen/page-coherent.h | 93 --
> arch/arm/mm/dma-mapping.c | 8 +-
> arch/arm64/include/asm/xen/page-coherent.h | 75 -
> drivers/xen/swiotlb-xen.c | 49 +---
> include/xen/arm/page-coherent.h| 80 +++
> 6 files changed, 83 insertions(+), 225 deletions(-)
>
> diff --git a/arch/arm/include/asm/device.h b/arch/arm/include/asm/device.h
> index f6955b55c544..c675bc0d5aa8 100644
> --- a/arch/arm/include/asm/device.h
> +++ b/arch/arm/include/asm/device.h
> @@ -14,9 +14,6 @@ struct dev_archdata {
> #endif
> #ifdef CONFIG_ARM_DMA_USE_IOMMU
> struct dma_iommu_mapping*mapping;
> -#endif
> -#ifdef CONFIG_XEN
> - const struct dma_map_ops *dev_dma_ops;
> #endif
> unsigned int dma_coherent:1;
> unsigned int dma_ops_setup:1;
> diff --git a/arch/arm/include/asm/xen/page-coherent.h
> b/arch/arm/include/asm/xen/page-coherent.h
> index 2c403e7c782d..27e984977402 100644
> --- a/arch/arm/include/asm/xen/page-coherent.h
> +++ b/arch/arm/include/asm/xen/page-coherent.h
> @@ -1,95 +1,2 @@
> /* SPDX-License-Identifier: GPL-2.0 */
> -#ifndef _ASM_ARM_XEN_PAGE_COHERENT_H
> -#define _ASM_ARM_XEN_PAGE_COHERENT_H
> -
> -#include
> -#include
> #include
> -
> -static inline const struct dma_map_ops *xen_get_dma_ops(struct device *dev)
> -{
> - if (dev && dev->archdata.dev_dma_ops)
> - return dev->archdata.dev_dma_ops;
> - return get_arch_dma_ops(NULL);
> -}
> -
> -static inline void *xen_alloc_coherent_pages(struct device *hwdev, size_t
> size,
> - dma_addr_t *dma_handle, gfp_t flags, unsigned long attrs)
> -{
> - return xen_get_dma_ops(hwdev)->alloc(hwdev, size, dma_handle, flags,
> attrs);
> -}
> -
> -static inline void xen_free_coherent_pages(struct device *hwdev, size_t size,
> - void *cpu_addr, dma_addr_t dma_handle, unsigned long attrs)
> -{
> - xen_get_dma_ops(hwdev)->free(hwdev, size, cpu_addr, dma_handle, attrs);
> -}
> -
> -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 page_pfn = page_to_xen_pfn(page);
> - unsigned long dev_pfn = XEN_PFN_DOWN(dev_addr);
> - unsigned long compound_pages =
> - (1< - bool local = (page_pfn <= dev_pfn) &&
> - (dev_pfn - page_pfn < compound_pages);
> -
> - /*
> - * Dom0 is mapped 1:1, while the Linux page can span across
> - * multiple Xen pages, it's not possible for it to contain a
> - * mix of local and foreign Xen pages. So if the first xen_pfn
> - * == mfn the page is local otherwise it's a foreign page
> - * grant-mapped in dom0. If the page is local we can safely
> - * call the native dma_ops function, otherwise we call the xen
> - * specific function.
> - */
> - if (local)
> - xen_get_dma_ops(hwdev)->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);
> - /*
> - * Dom0 is mapped 1:1, while the Linux page can be spanned accross
> - * multiple Xen page, it's not possible to have a mix of local and
> - * foreign Xen page. Dom0 is mapped 1:1, so calling pfn_valid on a
> - * foreign mfn will always return false. If the page is local we can
> - * safely call the native dma_ops function, otherwise we call the xen
> - * specific function.
> - */
> - if (pfn_valid(pfn)) {
> - if (xen_get_dma_ops(hwdev)->unmap_page)
> - xen_get_dma_ops(hwdev)->unmap_page(hwdev, handle, size,
> dir, attrs);
> - } else
> - __xen_dma_unmap_page(hwdev, handle, size, dir, attrs);
> -}
> -
> -static inline void xen_dma_sync_single_for_cpu(struct device *hwdev,
> - dma_addr_t handle,
Re: [PATCH 05/11] xen: remove the exports for xen_{create,destroy}_contiguous_region
On Mon, 26 Aug 2019, Christoph Hellwig wrote:
> These routines are only used by swiotlb-xen, which cannot be modular.
>
> Signed-off-by: Christoph Hellwig
Reviewed-by: Stefano Stabellini
> ---
> arch/arm/xen/mm.c | 2 --
> arch/x86/xen/mmu_pv.c | 2 --
> 2 files changed, 4 deletions(-)
>
> diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
> index 9b3a6c0ca681..b7d53415532b 100644
> --- a/arch/arm/xen/mm.c
> +++ b/arch/arm/xen/mm.c
> @@ -155,13 +155,11 @@ int xen_create_contiguous_region(phys_addr_t pstart,
> unsigned int order,
> *dma_handle = pstart;
> return 0;
> }
> -EXPORT_SYMBOL_GPL(xen_create_contiguous_region);
>
> void xen_destroy_contiguous_region(phys_addr_t pstart, unsigned int order)
> {
> return;
> }
> -EXPORT_SYMBOL_GPL(xen_destroy_contiguous_region);
>
> int __init xen_mm_init(void)
> {
> diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
> index 26e8b326966d..c8dbee62ec2a 100644
> --- a/arch/x86/xen/mmu_pv.c
> +++ b/arch/x86/xen/mmu_pv.c
> @@ -2625,7 +2625,6 @@ int xen_create_contiguous_region(phys_addr_t pstart,
> unsigned int order,
> *dma_handle = virt_to_machine(vstart).maddr;
> return success ? 0 : -ENOMEM;
> }
> -EXPORT_SYMBOL_GPL(xen_create_contiguous_region);
>
> void xen_destroy_contiguous_region(phys_addr_t pstart, unsigned int order)
> {
> @@ -2660,7 +2659,6 @@ void xen_destroy_contiguous_region(phys_addr_t pstart,
> unsigned int order)
>
> spin_unlock_irqrestore(_reservation_lock, flags);
> }
> -EXPORT_SYMBOL_GPL(xen_destroy_contiguous_region);
>
> static noinline void xen_flush_tlb_all(void)
> {
> --
> 2.20.1
>
Re: [PATCH 04/11] xen/arm: remove xen_dma_ops
On Mon, 26 Aug 2019, Christoph Hellwig wrote:
> arm and arm64 can just use xen_swiotlb_dma_ops directly like x86, no
> need for a pointer indirection.
>
> Signed-off-by: Christoph Hellwig
> Reviewed-by: Julien Grall
Reviewed-by: Stefano Stabellini
> ---
> arch/arm/mm/dma-mapping.c| 3 ++-
> arch/arm/xen/mm.c| 4
> arch/arm64/mm/dma-mapping.c | 3 ++-
> include/xen/arm/hypervisor.h | 2 --
> 4 files changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 738097396445..2661cad36359 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -35,6 +35,7 @@
> #include
> #include
> #include
> +#include
>
> #include "dma.h"
> #include "mm.h"
> @@ -2360,7 +2361,7 @@ void arch_setup_dma_ops(struct device *dev, u64
> dma_base, u64 size,
>
> #ifdef CONFIG_XEN
> if (xen_initial_domain())
> - dev->dma_ops = xen_dma_ops;
> + dev->dma_ops = _swiotlb_dma_ops;
> #endif
> dev->archdata.dma_ops_setup = true;
> }
> diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
> index 14210ebdea1a..9b3a6c0ca681 100644
> --- a/arch/arm/xen/mm.c
> +++ b/arch/arm/xen/mm.c
> @@ -163,16 +163,12 @@ void xen_destroy_contiguous_region(phys_addr_t pstart,
> unsigned int order)
> }
> EXPORT_SYMBOL_GPL(xen_destroy_contiguous_region);
>
> -const struct dma_map_ops *xen_dma_ops;
> -EXPORT_SYMBOL(xen_dma_ops);
> -
> int __init xen_mm_init(void)
> {
> struct gnttab_cache_flush cflush;
> if (!xen_initial_domain())
> return 0;
> xen_swiotlb_init(1, false);
> - xen_dma_ops = _swiotlb_dma_ops;
>
> cflush.op = 0;
> cflush.a.dev_bus_addr = 0;
> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> index bd2b039f43a6..4b244a037349 100644
> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch/arm64/mm/dma-mapping.c
> @@ -8,6 +8,7 @@
> #include
> #include
> #include
> +#include
>
> #include
>
> @@ -64,6 +65,6 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base,
> u64 size,
>
> #ifdef CONFIG_XEN
> if (xen_initial_domain())
> - dev->dma_ops = xen_dma_ops;
> + dev->dma_ops = _swiotlb_dma_ops;
> #endif
> }
> diff --git a/include/xen/arm/hypervisor.h b/include/xen/arm/hypervisor.h
> index 2982571f7cc1..43ef24dd030e 100644
> --- a/include/xen/arm/hypervisor.h
> +++ b/include/xen/arm/hypervisor.h
> @@ -19,8 +19,6 @@ static inline enum paravirt_lazy_mode
> paravirt_get_lazy_mode(void)
> return PARAVIRT_LAZY_NONE;
> }
>
> -extern const struct dma_map_ops *xen_dma_ops;
> -
> #ifdef CONFIG_XEN
> void __init xen_early_init(void);
> #else
> --
> 2.20.1
>
Re: swiotlb-xen cleanups
On Fri, 16 Aug 2019, Christoph Hellwig wrote: > Hi Xen maintainers and friends, > > please take a look at this series that cleans up the parts of swiotlb-xen > that deal with non-coherent caches. Hi Christoph, I just wanted to let you know that your series is on my radar, but I have been swamped the last few days. I hope to get to it by the end of the week. Cheers, Stefano
Re: [PATCH] swiotlb: fix phys_addr_t overflow warning
On Mon, 17 Jun 2019, Arnd Bergmann wrote:
> On architectures that have a larger dma_addr_t than phys_addr_t,
> the swiotlb_tbl_map_single() function truncates its return code
> in the failure path, making it impossible to identify the error
> later, as we compare to the original value:
>
> kernel/dma/swiotlb.c:551:9: error: implicit conversion from 'dma_addr_t' (aka
> 'unsigned long long') to 'phys_addr_t' (aka 'unsigned int') changes value
> from 18446744073709551615 to 4294967295 [-Werror,-Wconstant-conversion]
> return DMA_MAPPING_ERROR;
>
> Use an explicit typecast here to convert it to the narrower type,
> and use the same expression in the error handling later.
>
> Fixes: b907e20508d0 ("swiotlb: remove SWIOTLB_MAP_ERROR")
> Signed-off-by: Arnd Bergmann
Acked-by: Stefano Stabellini
> ---
> I still think that reverting the original commit would have
> provided clearer semantics for this corner case, but at least
> this patch restores the correct behavior.
> ---
> drivers/xen/swiotlb-xen.c | 2 +-
> kernel/dma/swiotlb.c | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index d53f3493a6b9..cfbe46785a3b 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -402,7 +402,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device
> *dev, struct page *page,
>
> map = swiotlb_tbl_map_single(dev, start_dma_addr, phys, size, dir,
>attrs);
> - if (map == DMA_MAPPING_ERROR)
> + if (map == (phys_addr_t)DMA_MAPPING_ERROR)
> return DMA_MAPPING_ERROR;
>
> dev_addr = xen_phys_to_bus(map);
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index e906ef2e6315..a3be651973ad 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -548,7 +548,7 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
> if (!(attrs & DMA_ATTR_NO_WARN) && printk_ratelimit())
> dev_warn(hwdev, "swiotlb buffer is full (sz: %zd bytes), total
> %lu (slots), used %lu (slots)\n",
>size, io_tlb_nslabs, tmp_io_tlb_used);
> - return DMA_MAPPING_ERROR;
> + return (phys_addr_t)DMA_MAPPING_ERROR;
> found:
> io_tlb_used += nslots;
> spin_unlock_irqrestore(_tlb_lock, flags);
> @@ -666,7 +666,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, dir, attrs);
> - if (*phys == DMA_MAPPING_ERROR)
> + if (*phys == (phys_addr_t)DMA_MAPPING_ERROR)
> return false;
>
> /* Ensure that the address returned is DMA'ble */
> --
> 2.20.0
>
Re: [PATCH v1] xen/swiotlb: rework early repeat code
On Fri, 24 May 2019, Sergey Dyasli wrote:
> Current repeat code is plain broken for the early=true case. Xen exchanges
> all DMA (<4GB) pages that it can on the first xen_swiotlb_fixup() attempt.
> All further attempts with a halved region will fail immediately because
> all DMA pages already belong to Dom0.
>
> Introduce contig_pages param for xen_swiotlb_fixup() to track the number
> of pages that were made contiguous in MFN space and use the same bootmem
> region while halving the memory requirements.
>
> Signed-off-by: Sergey Dyasli
Just FYI I am touching the same code to fix another unrelated bug, see:
https://marc.info/?l=xen-devel=155856767022893
> ---
> CC: Konrad Rzeszutek Wilk
> CC: Boris Ostrovsky
> CC: Juergen Gross
> CC: Stefano Stabellini
> CC: Paul Durrant
> ---
> drivers/xen/swiotlb-xen.c | 36 ++--
> 1 file changed, 30 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index 5dcb06fe9667..d2aba804d06c 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -142,7 +142,8 @@ static int is_xen_swiotlb_buffer(dma_addr_t dma_addr)
> static int max_dma_bits = 32;
>
> static int
> -xen_swiotlb_fixup(void *buf, size_t size, unsigned long nslabs)
> +xen_swiotlb_fixup(void *buf, size_t size, unsigned long nslabs,
> + unsigned long *contig_pages)
> {
> int i, rc;
> int dma_bits;
> @@ -156,10 +157,13 @@ xen_swiotlb_fixup(void *buf, size_t size, unsigned long
> nslabs)
> int slabs = min(nslabs - i, (unsigned long)IO_TLB_SEGSIZE);
>
> do {
> + unsigned int order = get_order(slabs << IO_TLB_SHIFT);
> rc = xen_create_contiguous_region(
> p + (i << IO_TLB_SHIFT),
> - get_order(slabs << IO_TLB_SHIFT),
> + order,
> dma_bits, _handle);
> + if (rc == 0)
> + *contig_pages += 1 << order;
> } while (rc && dma_bits++ < max_dma_bits);
> if (rc)
> return rc;
> @@ -202,7 +206,7 @@ static const char *xen_swiotlb_error(enum xen_swiotlb_err
> err)
> }
> int __ref xen_swiotlb_init(int verbose, bool early)
> {
> - unsigned long bytes, order;
> + unsigned long bytes, order, contig_pages;
> int rc = -ENOMEM;
> enum xen_swiotlb_err m_ret = XEN_SWIOTLB_UNKNOWN;
> unsigned int repeat = 3;
> @@ -244,13 +248,32 @@ int __ref xen_swiotlb_init(int verbose, bool early)
> /*
>* And replace that memory with pages under 4GB.
>*/
> + contig_pages = 0;
> rc = xen_swiotlb_fixup(xen_io_tlb_start,
> bytes,
> -xen_io_tlb_nslabs);
> +xen_io_tlb_nslabs,
> +_pages);
> if (rc) {
> - if (early)
> + if (early) {
> + unsigned long orig_bytes = bytes;
> + while (repeat-- > 0) {
> + xen_io_tlb_nslabs = max(1024UL, /* Min is 2MB */
> + (xen_io_tlb_nslabs >> 1));
> + pr_info("Lowering to %luMB\n",
> + (xen_io_tlb_nslabs << IO_TLB_SHIFT) >> 20);
> + bytes = xen_set_nslabs(xen_io_tlb_nslabs);
> + order = get_order(xen_io_tlb_nslabs <<
> IO_TLB_SHIFT);
> + xen_io_tlb_end = xen_io_tlb_start + bytes;
> + if (contig_pages >= (1ul << order)) {
> + /* Enough pages were made contiguous */
> + memblock_free(__pa(xen_io_tlb_start +
> bytes),
> + PAGE_ALIGN(orig_bytes -
> bytes));
> + goto fixup_done;
> + }
> + }
> memblock_free(__pa(xen_io_tlb_start),
> PAGE_ALIGN(bytes));
> + }
> else {
> free_pages((unsigned long)xen_io_tlb_start, order);
> xen_io_tlb_start = NULL;
> @@ -258,6 +281,7 @@ int __ref xen_swiotlb_init(int verbose, bool early)
> m_ret = XEN_SWIOTLB_EFIXUP;
> goto error;
> }
>
Re: [PATCH 3/3] xen/swiotlb: remember having called xen_create_contiguous_region()
On Tue, 23 Apr 2019, Juergen Gross wrote:
> Instead of always calling xen_destroy_contiguous_region() in case the
> memory is DMA-able for the used device, do so only in case it has been
> made DMA-able via xen_create_contiguous_region() before.
>
> This will avoid a lot of xen_destroy_contiguous_region() calls for
> 64-bit capable devices.
>
> As the memory in question is owned by swiotlb-xen the PG_owner_priv_1
> flag of the first allocated page can be used for remembering.
Although the patch looks OK, this sentence puzzles me. Why do you say
that the memory in question is owned by swiotlb-xen? Because it was
returned by xen_alloc_coherent_pages? Both the x86 and the Arm
implementation return fresh new memory, hence, it should be safe to set
the PageOwnerPriv1 flag?
My concern with this approach is with the semantics of PG_owner_priv_1.
Is a page marked with PG_owner_priv_1 only supposed to be used by the
owner?
> Signed-off-by: Juergen Gross
> ---
> drivers/xen/swiotlb-xen.c | 9 ++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index 43b6e65ae256..a72f181d8e20 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -321,6 +321,7 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t
> size,
> xen_free_coherent_pages(hwdev, size, ret,
> (dma_addr_t)phys, attrs);
> return NULL;
> }
> + SetPageOwnerPriv1(virt_to_page(ret));
> }
> memset(ret, 0, size);
> return ret;
> @@ -344,9 +345,11 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t
> size, void *vaddr,
> /* Convert the size to actually allocated. */
> size = 1UL << (order + XEN_PAGE_SHIFT);
>
> - if ((dev_addr + size - 1 <= dma_mask) &&
> - !WARN_ON(range_straddles_page_boundary(phys, size)))
> - xen_destroy_contiguous_region(phys, order);
> + if (PageOwnerPriv1(virt_to_page(vaddr))) {
> + if (!WARN_ON(range_straddles_page_boundary(phys, size)))
> + xen_destroy_contiguous_region(phys, order);
> + ClearPageOwnerPriv1(virt_to_page(vaddr));
> + }
>
> xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs);
> }
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 4/4] swiotlb-xen: ensure we have a single callsite for xen_dma_map_page
On Thu, 11 Apr 2019, Christoph Hellwig wrote:
> Refactor the code a bit to make further changes easier.
>
> Signed-off-by: Christoph Hellwig
Reviewed-by: Stefano Stabellini
> ---
> drivers/xen/swiotlb-xen.c | 31 ---
> 1 file changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index 9a951504dc12..5dcb06fe9667 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -391,13 +391,8 @@ static dma_addr_t xen_swiotlb_map_page(struct device
> *dev, struct page *page,
> if (dma_capable(dev, dev_addr, size) &&
> !range_straddles_page_boundary(phys, size) &&
> !xen_arch_need_swiotlb(dev, phys, dev_addr) &&
> - (swiotlb_force != SWIOTLB_FORCE)) {
> - /* 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);
> - return dev_addr;
> - }
> + swiotlb_force != SWIOTLB_FORCE)
> + goto done;
>
> /*
>* Oh well, have to allocate and map a bounce buffer.
> @@ -410,19 +405,25 @@ static dma_addr_t xen_swiotlb_map_page(struct device
> *dev, struct page *page,
> return DMA_MAPPING_ERROR;
>
> dev_addr = xen_phys_to_bus(map);
> - xen_dma_map_page(dev, pfn_to_page(map >> PAGE_SHIFT),
> - dev_addr, map & ~PAGE_MASK, size, dir,
> attrs);
>
> /*
>* Ensure that the address returned is DMA'ble
>*/
> - if (dma_capable(dev, dev_addr, size))
> - return dev_addr;
> -
> - attrs |= DMA_ATTR_SKIP_CPU_SYNC;
> - swiotlb_tbl_unmap_single(dev, map, size, dir, attrs);
> + if (unlikely(!dma_capable(dev, dev_addr, size))) {
> + swiotlb_tbl_unmap_single(dev, map, size, dir,
> + attrs | DMA_ATTR_SKIP_CPU_SYNC);
> + return DMA_MAPPING_ERROR;
> + }
>
> - 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);
> + return dev_addr;
> }
>
> /*
> --
> 2.20.1
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/4] swiotlb-xen: use ->map_page to implement ->map_sg
On Thu, 11 Apr 2019, Christoph Hellwig wrote:
> We can simply loop over the segments and map them, removing lots of
> duplicate code.
Right, the only difference is the additional dma_capable check which is
good to have.
Reviewed-by: Stefano Stabellini
> Signed-off-by: Christoph Hellwig
> ---
> drivers/xen/swiotlb-xen.c | 68 ++-
> 1 file changed, 10 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index d4bc3aabd44d..97a55c225593 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -517,24 +517,8 @@ xen_swiotlb_unmap_sg(struct device *hwdev, struct
> scatterlist *sgl, int nelems,
>
> }
>
> -/*
> - * Map a set of buffers described by scatterlist in streaming mode for DMA.
> - * This is the scatter-gather version of the above xen_swiotlb_map_page
> - * interface. Here the scatter gather list elements are each tagged with the
> - * appropriate dma address and length. They are obtained via
> - * sg_dma_{address,length}(SG).
> - *
> - * NOTE: An implementation may be able to use a smaller number of
> - * DMA address/length pairs than there are SG table elements.
> - * (for example via virtual mapping capabilities)
> - * The routine returns the number of addr/length pairs actually
> - * used, at most nents.
> - *
> - * Device ownership issues as mentioned above for xen_swiotlb_map_page are
> the
> - * same here.
> - */
> static int
> -xen_swiotlb_map_sg(struct device *hwdev, struct scatterlist *sgl, int nelems,
> +xen_swiotlb_map_sg(struct device *dev, struct scatterlist *sgl, int nelems,
> enum dma_data_direction dir, unsigned long attrs)
> {
> struct scatterlist *sg;
> @@ -543,50 +527,18 @@ xen_swiotlb_map_sg(struct device *hwdev, struct
> scatterlist *sgl, int nelems,
> BUG_ON(dir == DMA_NONE);
>
> for_each_sg(sgl, sg, nelems, i) {
> - phys_addr_t paddr = sg_phys(sg);
> - dma_addr_t dev_addr = xen_phys_to_bus(paddr);
> -
> - if (swiotlb_force == SWIOTLB_FORCE ||
> - xen_arch_need_swiotlb(hwdev, paddr, dev_addr) ||
> - !dma_capable(hwdev, dev_addr, sg->length) ||
> - range_straddles_page_boundary(paddr, sg->length)) {
> - phys_addr_t map = swiotlb_tbl_map_single(hwdev,
> - start_dma_addr,
> - sg_phys(sg),
> - sg->length,
> - dir, attrs);
> - if (map == DMA_MAPPING_ERROR) {
> - dev_warn(hwdev, "swiotlb buffer is full\n");
> - /* Don't panic here, we expect map_sg users
> -to do proper error handling. */
> - attrs |= DMA_ATTR_SKIP_CPU_SYNC;
> - xen_swiotlb_unmap_sg(hwdev, sgl, i, dir, attrs);
> - sg_dma_len(sgl) = 0;
> - return 0;
> - }
> - dev_addr = xen_phys_to_bus(map);
> - xen_dma_map_page(hwdev, pfn_to_page(map >> PAGE_SHIFT),
> - dev_addr,
> - map & ~PAGE_MASK,
> - sg->length,
> - dir,
> - attrs);
> - sg->dma_address = dev_addr;
> - } else {
> - /* 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(hwdev, pfn_to_page(paddr >>
> PAGE_SHIFT),
> - dev_addr,
> - paddr & ~PAGE_MASK,
> - sg->length,
> - dir,
> - attrs);
> - sg->dma_address = dev_addr;
> - }
> + sg->dma_address = xen_swiotlb_map_page(dev, sg_page(sg),
> + sg->offset, sg->length, dir, attrs);
> + if (sg->dma_address == DMA_MAPPING_ERROR)
> + go
Re: [PATCH 1/4] swiotlb-xen: make instances match their method names
On Thu, 11 Apr 2019, Christoph Hellwig wrote:
> Just drop two pointless _attrs prefixes to make the code a little
> more grep-able.
>
> Signed-off-by: Christoph Hellwig
Reviewed-by: Stefano Stabellini
> ---
> drivers/xen/swiotlb-xen.c | 17 +++--
> 1 file changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index 877baf2a94f4..d4bc3aabd44d 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -504,9 +504,8 @@ xen_swiotlb_sync_single_for_device(struct device *hwdev,
> dma_addr_t dev_addr,
> * concerning calls here are the same as for swiotlb_unmap_page() above.
> */
> static void
> -xen_swiotlb_unmap_sg_attrs(struct device *hwdev, struct scatterlist *sgl,
> -int nelems, enum dma_data_direction dir,
> -unsigned long attrs)
> +xen_swiotlb_unmap_sg(struct device *hwdev, struct scatterlist *sgl, int
> nelems,
> + enum dma_data_direction dir, unsigned long attrs)
> {
> struct scatterlist *sg;
> int i;
> @@ -535,9 +534,8 @@ xen_swiotlb_unmap_sg_attrs(struct device *hwdev, struct
> scatterlist *sgl,
> * same here.
> */
> static int
> -xen_swiotlb_map_sg_attrs(struct device *hwdev, struct scatterlist *sgl,
> - int nelems, enum dma_data_direction dir,
> - unsigned long attrs)
> +xen_swiotlb_map_sg(struct device *hwdev, struct scatterlist *sgl, int nelems,
> + enum dma_data_direction dir, unsigned long attrs)
> {
> struct scatterlist *sg;
> int i;
> @@ -562,8 +560,7 @@ xen_swiotlb_map_sg_attrs(struct device *hwdev, struct
> scatterlist *sgl,
> /* Don't panic here, we expect map_sg users
> to do proper error handling. */
> attrs |= DMA_ATTR_SKIP_CPU_SYNC;
> - xen_swiotlb_unmap_sg_attrs(hwdev, sgl, i, dir,
> -attrs);
> + xen_swiotlb_unmap_sg(hwdev, sgl, i, dir, attrs);
> sg_dma_len(sgl) = 0;
> return 0;
> }
> @@ -690,8 +687,8 @@ const struct dma_map_ops xen_swiotlb_dma_ops = {
> .sync_single_for_device = xen_swiotlb_sync_single_for_device,
> .sync_sg_for_cpu = xen_swiotlb_sync_sg_for_cpu,
> .sync_sg_for_device = xen_swiotlb_sync_sg_for_device,
> - .map_sg = xen_swiotlb_map_sg_attrs,
> - .unmap_sg = xen_swiotlb_unmap_sg_attrs,
> + .map_sg = xen_swiotlb_map_sg,
> + .unmap_sg = xen_swiotlb_unmap_sg,
> .map_page = xen_swiotlb_map_page,
> .unmap_page = xen_swiotlb_unmap_page,
> .dma_supported = xen_swiotlb_dma_supported,
> --
> 2.20.1
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/4] swiotlb-xen: simplify the DMA sync method implementations
On Thu, 11 Apr 2019, Christoph Hellwig wrote:
> Get rid of the grand multiplexer and implement the sync_single_for_cpu
> and sync_single_for_device methods directly, and then loop over them
> for the scatterlist based variants.
>
> Note that this also loses a few comments related to highlevel DMA API
> concepts, which have nothing to do with the swiotlb-xen implementation
> details.
>
> Signed-off-by: Christoph Hellwig
Reviewed-by: Stefano Stabellini
> ---
> drivers/xen/swiotlb-xen.c | 84 +--
> 1 file changed, 28 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index 97a55c225593..9a951504dc12 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -455,48 +455,28 @@ static void xen_swiotlb_unmap_page(struct device
> *hwdev, dma_addr_t dev_addr,
> xen_unmap_single(hwdev, dev_addr, size, dir, attrs);
> }
>
> -/*
> - * Make physical memory consistent for a single streaming mode DMA
> translation
> - * after a transfer.
> - *
> - * If you perform a xen_swiotlb_map_page() but wish to interrogate the buffer
> - * using the cpu, yet do not wish to teardown the dma mapping, you must
> - * call this function before doing so. At the next point you give the dma
> - * address back to the card, you must first perform a
> - * xen_swiotlb_dma_sync_for_device, and then the device again owns the buffer
> - */
> static void
> -xen_swiotlb_sync_single(struct device *hwdev, dma_addr_t dev_addr,
> - size_t size, enum dma_data_direction dir,
> - enum dma_sync_target target)
> +xen_swiotlb_sync_single_for_cpu(struct device *dev, dma_addr_t dma_addr,
> + size_t size, enum dma_data_direction dir)
> {
> - phys_addr_t paddr = xen_bus_to_phys(dev_addr);
> + phys_addr_t paddr = xen_bus_to_phys(dma_addr);
>
> - BUG_ON(dir == DMA_NONE);
> -
> - if (target == SYNC_FOR_CPU)
> - xen_dma_sync_single_for_cpu(hwdev, dev_addr, size, dir);
> + xen_dma_sync_single_for_cpu(dev, dma_addr, size, dir);
>
> - /* NOTE: We use dev_addr here, not paddr! */
> - if (is_xen_swiotlb_buffer(dev_addr))
> - swiotlb_tbl_sync_single(hwdev, paddr, size, dir, target);
> -
> - if (target == SYNC_FOR_DEVICE)
> - xen_dma_sync_single_for_device(hwdev, dev_addr, size, dir);
> + if (is_xen_swiotlb_buffer(dma_addr))
> + swiotlb_tbl_sync_single(dev, paddr, size, dir, SYNC_FOR_CPU);
> }
>
> -void
> -xen_swiotlb_sync_single_for_cpu(struct device *hwdev, dma_addr_t dev_addr,
> - size_t size, enum dma_data_direction dir)
> +static void
> +xen_swiotlb_sync_single_for_device(struct device *dev, dma_addr_t dma_addr,
> + size_t size, enum dma_data_direction dir)
> {
> - xen_swiotlb_sync_single(hwdev, dev_addr, size, dir, SYNC_FOR_CPU);
> -}
> + phys_addr_t paddr = xen_bus_to_phys(dma_addr);
>
> -void
> -xen_swiotlb_sync_single_for_device(struct device *hwdev, dma_addr_t dev_addr,
> -size_t size, enum dma_data_direction dir)
> -{
> - xen_swiotlb_sync_single(hwdev, dev_addr, size, dir, SYNC_FOR_DEVICE);
> + 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);
> }
>
> /*
> @@ -541,38 +521,30 @@ xen_swiotlb_map_sg(struct device *dev, struct
> scatterlist *sgl, int nelems,
> return 0;
> }
>
> -/*
> - * Make physical memory consistent for a set of streaming mode DMA
> translations
> - * after a transfer.
> - *
> - * The same as swiotlb_sync_single_* but for a scatter-gather list, same
> rules
> - * and usage.
> - */
> static void
> -xen_swiotlb_sync_sg(struct device *hwdev, struct scatterlist *sgl,
> - int nelems, enum dma_data_direction dir,
> - enum dma_sync_target target)
> +xen_swiotlb_sync_sg_for_cpu(struct device *dev, struct scatterlist *sgl,
> + int nelems, enum dma_data_direction dir)
> {
> struct scatterlist *sg;
> int i;
>
> - for_each_sg(sgl, sg, nelems, i)
> - xen_swiotlb_sync_single(hwdev, sg->dma_address,
> - sg_dma_len(sg), dir, target);
> -}
> -
> -static void
> -xen_swiotlb_sync_sg_for_cpu(struct device *hwdev, struct scatterlist *sg,
> - int nelems, enum dma_data_direction dir)
> -{
> - xen_swiotlb_sync_sg(hwdev, sg, nelems, dir, SYNC_FOR_CPU);
&g
Re: [PATCH] arm64/xen: fix xen-swiotlb cache flushing
On Sat, 19 Jan 2019, Christoph Hellwig wrote:
> [full quote deleted, please take a little more care when quoting]
>
> On Fri, Jan 18, 2019 at 04:44:23PM -0800, Stefano Stabellini wrote:
> > > #ifdef CONFIG_XEN
> > > - if (xen_initial_domain()) {
> > > - dev->archdata.dev_dma_ops = dev->dma_ops;
> > > + if (xen_initial_domain())
> > > dev->dma_ops = xen_dma_ops;
> > > - }
> > > #endif
> > > }
> >
> > This is an optional suggestion, but it would be nice to add a check on
> > dev->dma_ops being unset here, something like:
> >
> > #ifdef CONFIG_XEN
> > if (xen_initial_domain()) {
> > if (dev->dma_ops != NULL)
> > warning/error
> > dev->dma_ops = xen_dma_ops;
> > }
> >
> > Does it make sense?
>
> Well, no such check existed before, so this probably should be a
> separate patch if we care enough. I have a series for 5.1 pending
> that moves the IOMMU handling to the comment code which will make
> the ops assginment a lot cleaner, and I guess I could fold such
> a check in that. Doing it now will just create churn as it would
> have to get reworked anyway
Fine by me, thank you!
> > Reviewed-by: Stefano Stabellini
>
> Where should we pick this up? I could pick it up through the dma-mapping
> tree given that is where the problem is introduced, but the Xen or arm64
> trees would also fit.
I am happy for you to carry it in the dma-mapping tree, especially if
you have other fixes to send. Otherwise, let me know.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] arm64/xen: fix xen-swiotlb cache flushing
> 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)) {
> + if (xen_get_dma_ops(hwdev)->sync_single_for_device)
> + xen_get_dma_ops(hwdev)->sync_single_for_device(hwdev,
> handle, size, dir);
> + } else
> + __xen_dma_sync_single_for_device(hwdev, handle, size, dir);
> +}
> +
> +#endif /* _ASM_ARM_XEN_PAGE_COHERENT_H */
> diff --git a/arch/arm64/include/asm/device.h b/arch/arm64/include/asm/device.h
> index 3dd3d664c5c5..4658c937e173 100644
> --- a/arch/arm64/include/asm/device.h
> +++ b/arch/arm64/include/asm/device.h
> @@ -20,9 +20,6 @@ struct dev_archdata {
> #ifdef CONFIG_IOMMU_API
> void *iommu;/* private IOMMU data */
> #endif
> -#ifdef CONFIG_XEN
> - const struct dma_map_ops *dev_dma_ops;
> -#endif
> };
>
> struct pdev_archdata {
> diff --git a/arch/arm64/include/asm/xen/page-coherent.h
> b/arch/arm64/include/asm/xen/page-coherent.h
> index b3ef061d8b74..77e36decc50c 100644
> --- a/arch/arm64/include/asm/xen/page-coherent.h
> +++ b/arch/arm64/include/asm/xen/page-coherent.h
> @@ -1 +1,77 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_ARM64_XEN_PAGE_COHERENT_H
> +#define _ASM_ARM64_XEN_PAGE_COHERENT_H
> +
> +#include
> +#include
> #include
> +
> +static inline void *xen_alloc_coherent_pages(struct device *hwdev, size_t
> size,
> + dma_addr_t *dma_handle, gfp_t flags, unsigned long attrs)
> +{
> + return dma_direct_alloc(hwdev, size, dma_handle, flags, attrs);
> +}
> +
> +static inline void xen_free_coherent_pages(struct device *hwdev, size_t size,
> + void *cpu_addr, dma_addr_t dma_handle, unsigned long attrs)
> +{
> + dma_direct_free(hwdev, size, cpu_addr, dma_handle, 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)
> +{
> + 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 page_pfn = page_to_xen_pfn(page);
> + unsigned long dev_pfn = XEN_PFN_DOWN(dev_addr);
> + unsigned long compound_pages =
> + (1< + bool local = (page_pfn <= dev_pfn) &&
> + (dev_pfn - page_pfn < compound_pages);
> +
> + 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);
> + /*
> + * Dom0 is mapped 1:1, while the Linux page can be spanned accross
> + * multiple Xen page, it's not possible to have a mix of local and
> + * foreign Xen page. Dom0 is mapped 1:1, so calling pfn_valid on a
> + * foreign mfn will always return false. If the page is local we can
> + * safely call the native dma_ops function, otherwise we call the xen
> + * specific function.
> + */
> + if (pfn_valid(pfn))
> + dma_direct_unmap_page(hwdev, handle, size, dir, attrs);
> + else
> + __xen_dma_unmap_page(hwdev, handle, size, dir, attrs);
> +}
> +
> +#endif /* _ASM_ARM64_XEN_PAGE_COHERENT_H */
> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> index fb0908456a1f..78c0a72f822c 100644
> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch
Re: Fail to boot Dom0 on Xen Arm64 after "dma-mapping: bypass indirect calls for dma-direct"
On Thu, 17 Jan 2019, Christoph Hellwig wrote: > On Thu, Jan 17, 2019 at 11:43:49AM +, Julien Grall wrote: > > Looking at the change for arm64, you will always call dma-direct API. In > > previous Linux version, xen-swiotlb will call dev->archdata.dev_dma_ops (a > > copy of dev->dma_ops before setting Xen DMA ops) if not NULL. Does it mean > > we expect dev->dma_ops to always be NULL and hence using dma-direct API? > > The way I understood the code from inspecting it and sking the > maintainers a few askings is that for DOM0 we always use xen-swiotlb > as the actual dma_map_ops, but then use the functions in page-coherent.h > only to deal with cache maintainance, so it should be safe. Yes, what you wrote is correct, the stuff in include/xen/arm/page-coherent.h is only to deal with cache maintenance, specifically any cache maintenance operations that might be required by map/unmap_page, dma_sync_single_for_cpu/device. It looks like it is safe today to make the dma_direct calls directly, especially considering that on arm64 it looks like the only other option is iommu_dma_ops which has never been used with Xen so far (the IOMMU has not been exposed to guests yet). On arm32 we don't have this problem because dev->dma_ops is always != NULL with MMU enable (required on Xen), right? So the patch looks fine, I only have an optional suggestion to add a check on dma_ops being unset. I'll reply to the patch. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] dma-mapping: remove unused attrs parameter to dma_common_get_sgtable
On Thu, 3 Jan 2019, Huaisheng Ye wrote:
> From: Huaisheng Ye
>
> dma_common_get_sgtable has parameter attrs which is not used at all.
> Remove it.
>
> Signed-off-by: Huaisheng Ye
Acked-by: Stefano Stabellini
FYI the patch doesn't apply cleanly to master.
> ---
> drivers/xen/swiotlb-xen.c | 2 +-
> include/linux/dma-mapping.h | 5 ++---
> kernel/dma/mapping.c| 3 +--
> 3 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index 2a7f545..2dc17a5 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -697,7 +697,7 @@ static void xen_swiotlb_unmap_page(struct device *hwdev,
> dma_addr_t dev_addr,
> handle, size, attrs);
> }
> #endif
> - return dma_common_get_sgtable(dev, sgt, cpu_addr, handle, size, attrs);
> + return dma_common_get_sgtable(dev, sgt, cpu_addr, handle, size);
> }
>
> static int xen_swiotlb_mapping_error(struct device *dev, dma_addr_t dma_addr)
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index d327bdd..bbfad44 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -483,7 +483,7 @@ void *dma_common_pages_remap(struct page **pages, size_t
> size,
>
> int
> dma_common_get_sgtable(struct device *dev, struct sg_table *sgt, void
> *cpu_addr,
> - dma_addr_t dma_addr, size_t size, unsigned long attrs);
> + dma_addr_t dma_addr, size_t size);
>
> static inline int
> dma_get_sgtable_attrs(struct device *dev, struct sg_table *sgt, void
> *cpu_addr,
> @@ -495,8 +495,7 @@ void *dma_common_pages_remap(struct page **pages, size_t
> size,
> if (ops->get_sgtable)
> return ops->get_sgtable(dev, sgt, cpu_addr, dma_addr, size,
> attrs);
> - return dma_common_get_sgtable(dev, sgt, cpu_addr, dma_addr, size,
> - attrs);
> + return dma_common_get_sgtable(dev, sgt, cpu_addr, dma_addr, size);
> }
>
> #define dma_get_sgtable(d, t, v, h, s) dma_get_sgtable_attrs(d, t, v, h, s,
> 0)
> diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
> index 58dec7a..6b33f10 100644
> --- a/kernel/dma/mapping.c
> +++ b/kernel/dma/mapping.c
> @@ -202,8 +202,7 @@ void dmam_release_declared_memory(struct device *dev)
> * Create scatter-list for the already allocated DMA buffer.
> */
> int dma_common_get_sgtable(struct device *dev, struct sg_table *sgt,
> - void *cpu_addr, dma_addr_t dma_addr, size_t size,
> - unsigned long attrs)
> + void *cpu_addr, dma_addr_t dma_addr, size_t size)
> {
> struct page *page;
> int ret;
> --
> 1.8.3.1
>
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/2] swiotlb: Skip cache maintenance on map error
On Wed, 21 Nov 2018, Robin Murphy wrote:
> If swiotlb_bounce_page() failed, calling arch_sync_dma_for_device() may
> lead to such delights as performing cache maintenance on whatever
> address phys_to_virt(SWIOTLB_MAP_ERROR) looks like, which is typically
> outside the kernel memory map and goes about as well as expected.
>
> Don't do that.
>
> Fixes: a4a4330db46a ("swiotlb: add support for non-coherent DMA")
> Tested-by: John Stultz
> Reviewed-by: Christoph Hellwig
> Signed-off-by: Robin Murphy
Acked-by: Stefano Stabellini
> ---
>
> v2: Collect tags
>
> kernel/dma/swiotlb.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index 5731daa09a32..045930e32c0e 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -679,7 +679,8 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct
> page *page,
> }
>
> if (!dev_is_dma_coherent(dev) &&
> - (attrs & DMA_ATTR_SKIP_CPU_SYNC) == 0)
> + (attrs & DMA_ATTR_SKIP_CPU_SYNC) == 0 &&
> + dev_addr != DIRECT_MAPPING_ERROR)
> arch_sync_dma_for_device(dev, phys, size, dir);
>
> return dev_addr;
> --
> 2.19.1.dirty
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 0/2] SWIOTLB fixes for 4.20
On Tue, 27 Nov 2018, Stefano Stabellini wrote: > On Wed, 21 Nov 2018, Konrad Rzeszutek Wilk wrote: > > On Wed, Nov 21, 2018 at 02:03:31PM +0100, Christoph Hellwig wrote: > > > On Tue, Nov 20, 2018 at 11:34:41AM -0500, Konrad Rzeszutek Wilk wrote: > > > > > Konrad, are you ok with me picking up both through the dma-mapping > > > > > tree? > > > > > > > > Yes, albeit I would want Stefano to take a peek at patch #2 just in > > > > case. > > > > > > Stefano, can you take a look asap? This is a pretty trivial fix for > > > a nasty bug that breaks boot on real life systems. I'd like to merge > > > it by tomorrow so that I can send it off to Linus for the next rc > > > (I will be offline for about two days after Friday morning) > > > > It is Turkey Day in US so he may be busy catching those pesky turkeys. > > How about Tuesday? And in can also send the GIT PULL. > > I have just come back to the office. I'll give it a look today. Everything looks good to me, thanks for keeping me in the loop. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 0/2] SWIOTLB fixes for 4.20
On Wed, 21 Nov 2018, Konrad Rzeszutek Wilk wrote: > On Wed, Nov 21, 2018 at 02:03:31PM +0100, Christoph Hellwig wrote: > > On Tue, Nov 20, 2018 at 11:34:41AM -0500, Konrad Rzeszutek Wilk wrote: > > > > Konrad, are you ok with me picking up both through the dma-mapping > > > > tree? > > > > > > Yes, albeit I would want Stefano to take a peek at patch #2 just in case. > > > > Stefano, can you take a look asap? This is a pretty trivial fix for > > a nasty bug that breaks boot on real life systems. I'd like to merge > > it by tomorrow so that I can send it off to Linus for the next rc > > (I will be offline for about two days after Friday morning) > > It is Turkey Day in US so he may be busy catching those pesky turkeys. > How about Tuesday? And in can also send the GIT PULL. I have just come back to the office. I'll give it a look today. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH 0/5] arm64: IOMMU-backed DMA mapping
On Mon, 12 Jan 2015, Robin Murphy wrote: Hi all, Whilst it's a long way off perfect, this has reached the point of being functional and stable enough to be useful, so here it is. The core consists of the meat of the arch/arm implementation modified to remove the assumption of PAGE_SIZE pages and ported over to the Intel IOVA allocator instead of the bitmap-based one. For that, this series depends on my Genericise the IOVA allocator series posted earlier[1]. There are plenty of obvious things still to do, including: * Domain and group handling is all wrong, but that's a bigger problem. For the moment it does more or less the same thing as the arch/arm code, which at least works for the one-IOMMU-per-device situation. * IOMMU domains and IOVA domains probably want to be better integrated with devices and each other, rather than having a proliferation of arch-specific structs. * The temporary map_sg implementation - I have a 'proper' iommu_map_sg based one in progress, but since the simple one works it's not been as high a priority. * Port arch/arm over to it. I'd guess it might be preferable to merge this through arm64 first, though, rather than overcomplicate matters. * There may well be scope for streamlining and tidying up the copied parts - In general I've simply avoided touching anything I don't fully understand. * In the same vein, I'm sure lots of it is fairly ARM-specific, so will need longer-term work to become truly generic. [1]:http://thread.gmane.org/gmane.linux.kernel.iommu/8208 I tried to git-am and build a v3.19-rc4 kernel with this series (config file attached), but I get: In file included from include/linux/dma-mapping.h:82:0, from arch/arm64/kernel/asm-offsets.c:23: ./arch/arm64/include/asm/dma-mapping.h: In function ‘phys_to_dma’: ./arch/arm64/include/asm/dma-mapping.h:69:2: error: ‘struct dev_archdata’ has no member named ‘mapping’ ./arch/arm64/include/asm/dma-mapping.h: In function ‘dma_to_phys’: ./arch/arm64/include/asm/dma-mapping.h:81:19: error: ‘struct dev_archdata’ has no member named ‘mapping’ make[1]: *** [arch/arm64/kernel/asm-offsets.s] Error 1# # Automatically generated file; DO NOT EDIT. # Linux/arm64 3.19.0-rc4 Kernel Configuration # CONFIG_ARM64=y CONFIG_64BIT=y CONFIG_ARCH_PHYS_ADDR_T_64BIT=y CONFIG_MMU=y CONFIG_STACKTRACE_SUPPORT=y CONFIG_LOCKDEP_SUPPORT=y CONFIG_TRACE_IRQFLAGS_SUPPORT=y CONFIG_RWSEM_XCHGADD_ALGORITHM=y CONFIG_GENERIC_HWEIGHT=y CONFIG_GENERIC_CSUM=y CONFIG_GENERIC_CALIBRATE_DELAY=y CONFIG_ZONE_DMA=y CONFIG_HAVE_GENERIC_RCU_GUP=y CONFIG_ARCH_DMA_ADDR_T_64BIT=y CONFIG_NEED_DMA_MAP_STATE=y CONFIG_NEED_SG_DMA_LENGTH=y CONFIG_SWIOTLB=y CONFIG_IOMMU_HELPER=y CONFIG_KERNEL_MODE_NEON=y CONFIG_FIX_EARLYCON_MEM=y CONFIG_DEFCONFIG_LIST=/lib/modules/$UNAME_RELEASE/.config CONFIG_IRQ_WORK=y CONFIG_BUILDTIME_EXTABLE_SORT=y # # General setup # CONFIG_INIT_ENV_ARG_LIMIT=32 CONFIG_CROSS_COMPILE= # CONFIG_COMPILE_TEST is not set CONFIG_LOCALVERSION= # CONFIG_LOCALVERSION_AUTO is not set CONFIG_DEFAULT_HOSTNAME=(none) CONFIG_SWAP=y CONFIG_SYSVIPC=y CONFIG_SYSVIPC_SYSCTL=y # CONFIG_POSIX_MQUEUE is not set CONFIG_CROSS_MEMORY_ATTACH=y # CONFIG_FHANDLE is not set CONFIG_USELIB=y # CONFIG_AUDIT is not set CONFIG_HAVE_ARCH_AUDITSYSCALL=y # # IRQ subsystem # CONFIG_GENERIC_IRQ_PROBE=y CONFIG_GENERIC_IRQ_SHOW=y CONFIG_HARDIRQS_SW_RESEND=y CONFIG_IRQ_DOMAIN=y CONFIG_IRQ_DOMAIN_HIERARCHY=y CONFIG_GENERIC_MSI_IRQ=y CONFIG_GENERIC_MSI_IRQ_DOMAIN=y CONFIG_HANDLE_DOMAIN_IRQ=y # CONFIG_IRQ_DOMAIN_DEBUG is not set CONFIG_SPARSE_IRQ=y CONFIG_GENERIC_TIME_VSYSCALL=y CONFIG_GENERIC_CLOCKEVENTS=y CONFIG_GENERIC_CLOCKEVENTS_BUILD=y CONFIG_ARCH_HAS_TICK_BROADCAST=y CONFIG_GENERIC_CLOCKEVENTS_BROADCAST=y # # Timers subsystem # CONFIG_TICK_ONESHOT=y CONFIG_NO_HZ_COMMON=y # CONFIG_HZ_PERIODIC is not set CONFIG_NO_HZ_IDLE=y # CONFIG_NO_HZ_FULL is not set CONFIG_NO_HZ=y CONFIG_HIGH_RES_TIMERS=y # # CPU/Task time and stats accounting # CONFIG_TICK_CPU_ACCOUNTING=y # CONFIG_VIRT_CPU_ACCOUNTING_GEN is not set # CONFIG_BSD_PROCESS_ACCT is not set # CONFIG_TASKSTATS is not set # # RCU Subsystem # CONFIG_TREE_RCU=y # CONFIG_TASKS_RCU is not set CONFIG_RCU_STALL_COMMON=y # CONFIG_RCU_USER_QS is not set CONFIG_RCU_FANOUT=32 CONFIG_RCU_FANOUT_LEAF=16 # CONFIG_RCU_FANOUT_EXACT is not set # CONFIG_RCU_FAST_NO_HZ is not set # CONFIG_TREE_RCU_TRACE is not set # CONFIG_RCU_NOCB_CPU is not set # CONFIG_BUILD_BIN2C is not set # CONFIG_IKCONFIG is not set CONFIG_LOG_BUF_SHIFT=14 CONFIG_LOG_CPU_MAX_BUF_SHIFT=12 CONFIG_GENERIC_SCHED_CLOCK=y # CONFIG_CGROUPS is not set # CONFIG_CHECKPOINT_RESTORE is not set CONFIG_NAMESPACES=y CONFIG_UTS_NS=y CONFIG_IPC_NS=y # CONFIG_USER_NS is not set CONFIG_PID_NS=y CONFIG_NET_NS=y # CONFIG_SCHED_AUTOGROUP is not set # CONFIG_SYSFS_DEPRECATED is not set # CONFIG_RELAY is not set CONFIG_BLK_DEV_INITRD=y CONFIG_INITRAMFS_SOURCE= CONFIG_RD_GZIP=y # CONFIG_RD_BZIP2 is not set # CONFIG_RD_LZMA
Re: [RFC] add a struct page* parameter to dma_map_ops.unmap_page
On Mon, 17 Nov 2014, Stefano Stabellini wrote:
Hi all,
I am writing this email to ask for your advice.
On architectures where dma addresses are different from physical
addresses, it can be difficult to retrieve the physical address of a
page from its dma address.
Specifically this is the case for Xen on arm and arm64 but I think that
other architectures might have the same issue.
Knowing the physical address is necessary to be able to issue any
required cache maintenance operations when unmap_page,
sync_single_for_cpu and sync_single_for_device are called.
Adding a struct page* parameter to unmap_page, sync_single_for_cpu and
sync_single_for_device would make Linux dma handling on Xen on arm and
arm64 much easier and quicker.
I think that other drivers have similar problems, such as the Intel
IOMMU driver having to call find_iova and walking down an rbtree to get
the physical address in its implementation of unmap_page.
Callers have the struct page* in their hands already from the previous
map_page call so it shouldn't be an issue for them. A problem does
exist however: there are about 280 callers of dma_unmap_page and
pci_unmap_page. We have even more callers of the dma_sync_single_for_*
functions.
Is such a change even conceivable? How would one go about it?
I think that Xen would not be the only one to gain from it, but I would
like to have a confirmation from others: given the magnitude of the
changes involved I would actually prefer to avoid them unless multiple
drivers/archs/subsystems could really benefit from them.
Given the lack of interest from the community, I am going to drop this
idea.
Cheers,
Stefano
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index d5d3881..158a765 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -31,8 +31,9 @@ struct dma_map_ops {
unsigned long offset, size_t size,
enum dma_data_direction dir,
struct dma_attrs *attrs);
- void (*unmap_page)(struct device *dev, dma_addr_t dma_handle,
-size_t size, enum dma_data_direction dir,
+ void (*unmap_page)(struct device *dev, struct page *page,
+dma_addr_t dma_handle, size_t size,
+enum dma_data_direction dir,
struct dma_attrs *attrs);
int (*map_sg)(struct device *dev, struct scatterlist *sg,
int nents, enum dma_data_direction dir,
@@ -41,10 +42,10 @@ struct dma_map_ops {
struct scatterlist *sg, int nents,
enum dma_data_direction dir,
struct dma_attrs *attrs);
- void (*sync_single_for_cpu)(struct device *dev,
+ void (*sync_single_for_cpu)(struct device *dev, struct page *page,
dma_addr_t dma_handle, size_t size,
enum dma_data_direction dir);
- void (*sync_single_for_device)(struct device *dev,
+ void (*sync_single_for_device)(struct device *dev, struct page *page,
dma_addr_t dma_handle, size_t size,
enum dma_data_direction dir);
void (*sync_sg_for_cpu)(struct device *dev,
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[RFC] add a struct page* parameter to dma_map_ops.unmap_page
Hi all,
I am writing this email to ask for your advice.
On architectures where dma addresses are different from physical
addresses, it can be difficult to retrieve the physical address of a
page from its dma address.
Specifically this is the case for Xen on arm and arm64 but I think that
other architectures might have the same issue.
Knowing the physical address is necessary to be able to issue any
required cache maintenance operations when unmap_page,
sync_single_for_cpu and sync_single_for_device are called.
Adding a struct page* parameter to unmap_page, sync_single_for_cpu and
sync_single_for_device would make Linux dma handling on Xen on arm and
arm64 much easier and quicker.
I think that other drivers have similar problems, such as the Intel
IOMMU driver having to call find_iova and walking down an rbtree to get
the physical address in its implementation of unmap_page.
Callers have the struct page* in their hands already from the previous
map_page call so it shouldn't be an issue for them. A problem does
exist however: there are about 280 callers of dma_unmap_page and
pci_unmap_page. We have even more callers of the dma_sync_single_for_*
functions.
Is such a change even conceivable? How would one go about it?
I think that Xen would not be the only one to gain from it, but I would
like to have a confirmation from others: given the magnitude of the
changes involved I would actually prefer to avoid them unless multiple
drivers/archs/subsystems could really benefit from them.
Cheers,
Stefano
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index d5d3881..158a765 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -31,8 +31,9 @@ struct dma_map_ops {
unsigned long offset, size_t size,
enum dma_data_direction dir,
struct dma_attrs *attrs);
- void (*unmap_page)(struct device *dev, dma_addr_t dma_handle,
- size_t size, enum dma_data_direction dir,
+ void (*unmap_page)(struct device *dev, struct page *page,
+ dma_addr_t dma_handle, size_t size,
+ enum dma_data_direction dir,
struct dma_attrs *attrs);
int (*map_sg)(struct device *dev, struct scatterlist *sg,
int nents, enum dma_data_direction dir,
@@ -41,10 +42,10 @@ struct dma_map_ops {
struct scatterlist *sg, int nents,
enum dma_data_direction dir,
struct dma_attrs *attrs);
- void (*sync_single_for_cpu)(struct device *dev,
+ void (*sync_single_for_cpu)(struct device *dev, struct page *page,
dma_addr_t dma_handle, size_t size,
enum dma_data_direction dir);
- void (*sync_single_for_device)(struct device *dev,
+ void (*sync_single_for_device)(struct device *dev, struct page *page,
dma_addr_t dma_handle, size_t size,
enum dma_data_direction dir);
void (*sync_sg_for_cpu)(struct device *dev,
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
