diff mbox

[2/3] swiotlb: prefix dma_to_phys and phys_to_dma functions

Message ID 56EC1805.5060207@codeaurora.org (mailing list archive)
State New, archived
Headers show

Commit Message

Sinan Kaya March 18, 2016, 3 p.m. UTC
On 3/18/2016 8:12 AM, Robin Murphy wrote:
> Since we know for sure that swiotlb_to_phys is a no-op on arm64, it might be cleaner to simply not reference it at all. I suppose we could have some private local wrappers, e.g.:
> 
> #define swiotlb_to_virt(addr) phys_to_virt((phys_addr_t)(addr))
> 
> to keep the intent of the code clear (and just in case anyone ever builds a system mad enough to warrant switching out that definition, but I'd hope that never happens).
> 
> Otherwise, looks good - thanks for doing this!

OK. I added this. Reviewed-by?

I'm not happy to submit such a big patch for all different ARCHs. I couldn't
find a cleaner solution. I'm willing to split this patch into multiple if there
is a better way.

Comments

Konrad Rzeszutek Wilk March 28, 2016, 6:29 p.m. UTC | #1
On Fri, Mar 18, 2016 at 11:00 AM, Sinan Kaya <okaya@codeaurora.org> wrote:
> On 3/18/2016 8:12 AM, Robin Murphy wrote:
>> Since we know for sure that swiotlb_to_phys is a no-op on arm64, it might be cleaner to simply not reference it at all. I suppose we could have some private local wrappers, e.g.:
>>
>> #define swiotlb_to_virt(addr) phys_to_virt((phys_addr_t)(addr))
>>
>> to keep the intent of the code clear (and just in case anyone ever builds a system mad enough to warrant switching out that definition, but I'd hope that never happens).
>>
>> Otherwise, looks good - thanks for doing this!
>
> OK. I added this. Reviewed-by?
>
> I'm not happy to submit such a big patch for all different ARCHs. I couldn't
> find a cleaner solution. I'm willing to split this patch into multiple if there
> is a better way.
>
> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> index ada00c3..8c0f66b 100644
> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch/arm64/mm/dma-mapping.c
> @@ -29,6 +29,14 @@
>
>  #include <asm/cacheflush.h>
>
> +/*
> + * If you are building a system without IOMMU, then you are using SWIOTLB
> + * library. The ARM64 adaptation of this library does not support address
> + * translation and it assumes that physical address = dma address for such
> + * a use case. Please don't build a platform that violates this.
> + */

Why not just expand the ARM64 part to support address translation?

> +#define swiotlb_to_virt(addr) phys_to_virt((phys_addr_t)(addr))
> +

Adding Stefano here.

>  static pgprot_t __get_dma_pgprot(struct dma_attrs *attrs, pgprot_t prot,
>                                  bool coherent)
>  {
> @@ -188,7 +196,7 @@ static void __dma_free(struct device *dev, size_t size,
>                        void *vaddr, dma_addr_t dma_handle,
>                        struct dma_attrs *attrs)
>  {
> -       void *swiotlb_addr = phys_to_virt(swiotlb_dma_to_phys(dev, dma_handle));
> +       void *swiotlb_addr = swiotlb_to_virt(dma_handle);
>
>         size = PAGE_ALIGN(size);
>
> @@ -209,8 +217,7 @@ static dma_addr_t __swiotlb_map_page(struct device *dev, struct page *page,
>
>         dev_addr = swiotlb_map_page(dev, page, offset, size, dir, attrs);
>         if (!is_device_dma_coherent(dev))
> -               __dma_map_area(phys_to_virt(swiotlb_dma_to_phys(dev, dev_addr)),
> -                              size, dir);
> +               __dma_map_area(swiotlb_to_virt(dev_addr), size, dir);
>
>         return dev_addr;
>  }
> @@ -283,8 +290,7 @@ static void __swiotlb_sync_single_for_device(struct device *dev,
>  {
>         swiotlb_sync_single_for_device(dev, dev_addr, size, dir);
>         if (!is_device_dma_coherent(dev))
> -               __dma_map_area(phys_to_virt(swiotlb_dma_to_phys(dev, dev_addr)),
> -                              size, dir);
> +               __dma_map_area(swiotlb_to_virt(dev_addr), size, dir);
>  }
>
>
>
>
> --
> Sinan Kaya
> Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Stefano Stabellini March 29, 2016, 12:44 p.m. UTC | #2
On Mon, 28 Mar 2016, Konrad Rzeszutek Wilk wrote:
> On Fri, Mar 18, 2016 at 11:00 AM, Sinan Kaya <okaya@codeaurora.org> wrote:
> > On 3/18/2016 8:12 AM, Robin Murphy wrote:
> >> Since we know for sure that swiotlb_to_phys is a no-op on arm64, it might be cleaner to simply not reference it at all. I suppose we could have some private local wrappers, e.g.:
> >>
> >> #define swiotlb_to_virt(addr) phys_to_virt((phys_addr_t)(addr))
> >>
> >> to keep the intent of the code clear (and just in case anyone ever builds a system mad enough to warrant switching out that definition, but I'd hope that never happens).
> >>
> >> Otherwise, looks good - thanks for doing this!
> >
> > OK. I added this. Reviewed-by?
> >
> > I'm not happy to submit such a big patch for all different ARCHs. I couldn't
> > find a cleaner solution. I'm willing to split this patch into multiple if there
> > is a better way.
> >
> > diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> > index ada00c3..8c0f66b 100644
> > --- a/arch/arm64/mm/dma-mapping.c
> > +++ b/arch/arm64/mm/dma-mapping.c
> > @@ -29,6 +29,14 @@
> >
> >  #include <asm/cacheflush.h>
> >
> > +/*
> > + * If you are building a system without IOMMU, then you are using SWIOTLB
> > + * library. The ARM64 adaptation of this library does not support address
> > + * translation and it assumes that physical address = dma address for such
> > + * a use case. Please don't build a platform that violates this.
> > + */
> 
> Why not just expand the ARM64 part to support address translation?
>
> > +#define swiotlb_to_virt(addr) phys_to_virt((phys_addr_t)(addr))
> > +
> 
> Adding Stefano here.

Could you please explain what is the problem that you are trying to
solve? In other words, what is the issue with assuming that physical
address = dma address (and the current dma_to_phys and phys_to_dma
static inlines) if no arm64 platforms violate it? That's pretty much
what is done on x86 too (without X86_DMA_REMAP).

If you want to make sure that the assumption is not violated, you can
introduce a boot time check or a BUG_ON somewhere.

If there is an arm64 platform with phys_addr != dma_addr, we need proper
support for it. In fact even if there is an IOMMU on that platform, when
running Xen on it, the IOMMU would be used by the hypervisor and Linux
would still end up without it, using the swiotlb.

 
> >  static pgprot_t __get_dma_pgprot(struct dma_attrs *attrs, pgprot_t prot,
> >                                  bool coherent)
> >  {
> > @@ -188,7 +196,7 @@ static void __dma_free(struct device *dev, size_t size,
> >                        void *vaddr, dma_addr_t dma_handle,
> >                        struct dma_attrs *attrs)
> >  {
> > -       void *swiotlb_addr = phys_to_virt(swiotlb_dma_to_phys(dev, dma_handle));
> > +       void *swiotlb_addr = swiotlb_to_virt(dma_handle);
> >
> >         size = PAGE_ALIGN(size);
> >
> > @@ -209,8 +217,7 @@ static dma_addr_t __swiotlb_map_page(struct device *dev, struct page *page,
> >
> >         dev_addr = swiotlb_map_page(dev, page, offset, size, dir, attrs);
> >         if (!is_device_dma_coherent(dev))
> > -               __dma_map_area(phys_to_virt(swiotlb_dma_to_phys(dev, dev_addr)),
> > -                              size, dir);
> > +               __dma_map_area(swiotlb_to_virt(dev_addr), size, dir);
> >
> >         return dev_addr;
> >  }
> > @@ -283,8 +290,7 @@ static void __swiotlb_sync_single_for_device(struct device *dev,
> >  {
> >         swiotlb_sync_single_for_device(dev, dev_addr, size, dir);
> >         if (!is_device_dma_coherent(dev))
> > -               __dma_map_area(phys_to_virt(swiotlb_dma_to_phys(dev, dev_addr)),
> > -                              size, dir);
> > +               __dma_map_area(swiotlb_to_virt(dev_addr), size, dir);
> >  }
> >
> >
> >
> >
> > --
> > Sinan Kaya
> > Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
> > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
>
Sinan Kaya March 29, 2016, 12:57 p.m. UTC | #3
On 3/29/2016 8:44 AM, Stefano Stabellini wrote:
> Could you please explain what is the problem that you are trying to
> solve? In other words, what is the issue with assuming that physical
> address = dma address (and the current dma_to_phys and phys_to_dma
> static inlines) if no arm64 platforms violate it? That's pretty much
> what is done on x86 too (without X86_DMA_REMAP).
> 
> If you want to make sure that the assumption is not violated, you can
> introduce a boot time check or a BUG_ON somewhere.
> 
> If there is an arm64 platform with phys_addr != dma_addr, we need proper
> support for it. In fact even if there is an IOMMU on that platform, when
> running Xen on it, the IOMMU would be used by the hypervisor and Linux
> would still end up without it, using the swiotlb.


The problem is that device drivers are trying to use dma_to_phys and phys_to_dma
APIs to do address translation even though these two API do not exist in DMA mapping
layer. (see patch #1 and I made the same mistake in my HIDMA code)

Especially, when a device is behind an IOMMU; the DMA addresses are not equal
to physical addresses. Usage of dma_to_phys causes a crash on the system.

I'm trying to prefix the dma_to_phys and phys_to_dma API with swiotlb so that
we know what it is intended for and usage of these API in drivers need to be
discouraged in the future during code reviews.

Since I renamed the API, Robin Murphy made a suggestion to convert this 

phys_to_virt(swiotlb_dma_to_phys(dev, dma_handle))

to this

#define swiotlb_to_virt(addr) phys_to_virt((phys_addr_t)(addr))

swiotlb_to_virt(dma_handle)

just to reduce code clutter since we know swiotlb_dma_to_phys returns phys already
for ARM64 architecture.

I think we can do this exercise later. I'll undo this change for the moment. 
Let's focus on the API rename. 

I don't want this general purpose dma_to_phys API returning phys=dma value. This is
not correct.
Arnd Bergmann March 29, 2016, 7:32 p.m. UTC | #4
On Monday 28 March 2016 14:29:29 Konrad Rzeszutek Wilk wrote:
> > diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> > index ada00c3..8c0f66b 100644
> > --- a/arch/arm64/mm/dma-mapping.c
> > +++ b/arch/arm64/mm/dma-mapping.c
> > @@ -29,6 +29,14 @@
> >
> >  #include <asm/cacheflush.h>
> >
> > +/*
> > + * If you are building a system without IOMMU, then you are using SWIOTLB
> > + * library. The ARM64 adaptation of this library does not support address
> > + * translation and it assumes that physical address = dma address for such
> > + * a use case. Please don't build a platform that violates this.
> > + */
> 
> Why not just expand the ARM64 part to support address translation?

Because so far all hardware we have is relatively sane. We only
need to implement this if someone accidentally puts their DMA
space at the wrong address.

There is at least one platform that could in theory use this because
their RAM starts at an address that is outside of the reach of 32-bit
devices, and a static IOMMU mapping (created by firmware) could be
used to map the start of RAM into DMA address zero, to avoid having
to use an IOMMU all the time, but it was considered not worth the
effort to implement that.

	Arnd
diff mbox

Patch

diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index ada00c3..8c0f66b 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -29,6 +29,14 @@ 

 #include <asm/cacheflush.h>

+/*
+ * If you are building a system without IOMMU, then you are using SWIOTLB
+ * library. The ARM64 adaptation of this library does not support address
+ * translation and it assumes that physical address = dma address for such
+ * a use case. Please don't build a platform that violates this.
+ */
+#define swiotlb_to_virt(addr) phys_to_virt((phys_addr_t)(addr))
+
 static pgprot_t __get_dma_pgprot(struct dma_attrs *attrs, pgprot_t prot,
                                 bool coherent)
 {
@@ -188,7 +196,7 @@  static void __dma_free(struct device *dev, size_t size,
                       void *vaddr, dma_addr_t dma_handle,
                       struct dma_attrs *attrs)
 {
-       void *swiotlb_addr = phys_to_virt(swiotlb_dma_to_phys(dev, dma_handle));
+       void *swiotlb_addr = swiotlb_to_virt(dma_handle);

        size = PAGE_ALIGN(size);

@@ -209,8 +217,7 @@  static dma_addr_t __swiotlb_map_page(struct device *dev, struct page *page,

        dev_addr = swiotlb_map_page(dev, page, offset, size, dir, attrs);
        if (!is_device_dma_coherent(dev))
-               __dma_map_area(phys_to_virt(swiotlb_dma_to_phys(dev, dev_addr)),
-                              size, dir);
+               __dma_map_area(swiotlb_to_virt(dev_addr), size, dir);

        return dev_addr;
 }
@@ -283,8 +290,7 @@  static void __swiotlb_sync_single_for_device(struct device *dev,
 {
        swiotlb_sync_single_for_device(dev, dev_addr, size, dir);
        if (!is_device_dma_coherent(dev))
-               __dma_map_area(phys_to_virt(swiotlb_dma_to_phys(dev, dev_addr)),
-                              size, dir);
+               __dma_map_area(swiotlb_to_virt(dev_addr), size, dir);
 }