Re: [PATCH v5 2/3] iommu/dma: Reserve IOVA for PCIe inaccessible DMA address
Hi Robin, Lorenzo,
Thanks for review and guidance.
AFAIU, conclusion of discussion is, to return error if dma-ranges list
is not sorted.
So that, Can I send a new patch with below change to return error if
dma-ranges list is not sorted?
-static void iova_reserve_pci_windows(struct pci_dev *dev,
+static int iova_reserve_pci_windows(struct pci_dev *dev,
struct iova_domain *iovad)
{
struct pci_host_bridge *bridge = pci_find_host_bridge(dev->bus);
@@ -227,11 +227,15 @@ static void iova_reserve_pci_windows(struct pci_dev *dev,
resource_list_for_each_entry(window, >dma_ranges) {
end = window->res->start - window->offset;
resv_iova:
- if (end - start) {
+ if (end > start) {
lo = iova_pfn(iovad, start);
hi = iova_pfn(iovad, end);
reserve_iova(iovad, lo, hi);
+ } else {
+ dev_err(>dev, "Unsorted dma_ranges list\n");
+ return -EINVAL;
}
+
Please provide your inputs if any more changes required. Thank you,
Regards,
Srinath.
On Thu, May 2, 2019 at 7:45 PM Robin Murphy wrote:
>
> On 02/05/2019 14:06, Lorenzo Pieralisi wrote:
> > On Thu, May 02, 2019 at 12:27:02PM +0100, Robin Murphy wrote:
> >> Hi Lorenzo,
> >>
> >> On 02/05/2019 12:01, Lorenzo Pieralisi wrote:
> >>> On Wed, May 01, 2019 at 11:06:25PM +0530, Srinath Mannam wrote:
> dma_ranges field of PCI host bridge structure has resource entries in
> sorted order of address range given through dma-ranges DT property. This
> list is the accessible DMA address range. So that this resource list will
> be processed and reserve IOVA address to the inaccessible address holes
> in
> the list.
>
> This method is similar to PCI IO resources address ranges reserving in
> IOMMU for each EP connected to host bridge.
>
> Signed-off-by: Srinath Mannam
> Based-on-patch-by: Oza Pawandeep
> Reviewed-by: Oza Pawandeep
> Acked-by: Robin Murphy
> ---
> drivers/iommu/dma-iommu.c | 19 +++
> 1 file changed, 19 insertions(+)
>
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 77aabe6..da94844 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -212,6 +212,7 @@ static void iova_reserve_pci_windows(struct pci_dev
> *dev,
> struct pci_host_bridge *bridge =
> pci_find_host_bridge(dev->bus);
> struct resource_entry *window;
> unsigned long lo, hi;
> + phys_addr_t start = 0, end;
> resource_list_for_each_entry(window, >windows) {
> if (resource_type(window->res) != IORESOURCE_MEM)
> @@ -221,6 +222,24 @@ static void iova_reserve_pci_windows(struct pci_dev
> *dev,
> hi = iova_pfn(iovad, window->res->end -
> window->offset);
> reserve_iova(iovad, lo, hi);
> }
> +
> + /* Get reserved DMA windows from host bridge */
> + resource_list_for_each_entry(window, >dma_ranges) {
> >>>
> >>> If this list is not sorted it seems to me the logic in this loop is
> >>> broken and you can't rely on callers to sort it because it is not a
> >>> written requirement and it is not enforced (you know because you
> >>> wrote the code but any other developer is not supposed to guess
> >>> it).
> >>>
> >>> Can't we rewrite this loop so that it does not rely on list
> >>> entries order ?
> >>
> >> The original idea was that callers should be required to provide a sorted
> >> list, since it keeps things nice and simple...
> >
> > I understand, if it was self-contained in driver code that would be fine
> > but in core code with possible multiple consumers this must be
> > documented/enforced, somehow.
> >
> >>> I won't merge this series unless you sort it, no pun intended.
> >>>
> >>> Lorenzo
> >>>
> + end = window->res->start - window->offset;
> >>
> >> ...so would you consider it sufficient to add
> >>
> >> if (end < start)
> >> dev_err(...);
> >
> > We should also revert any IOVA reservation we did prior to this
> > error, right ?
>
> I think it would be enough to propagate an error code back out through
> iommu_dma_init_domain(), which should then end up aborting the whole
> IOMMU setup - reserve_iova() isn't really designed to be undoable, but
> since this is the kind of error that should only ever be hit during
> driver or DT development, as long as we continue booting such that the
> developer can clearly see what's gone wrong, I don't think we need
> bother spending too much effort tidying up inside the unused domain.
>
> > Anyway, I think it is best to ensure it *is* sorted.
> >
> >> here, plus commenting the definition of pci_host_bridge::dma_ranges
Re: [PATCH v4 0/3] PCIe Host request to reserve IOVA
Hi David, Thanks for review, I will fix in next version of this patch set. Regards, Srinath. On Thu, May 2, 2019 at 3:24 PM David Laight wrote: > > From: Srinath Mannam > > Sent: 01 May 2019 16:23 > ... > > > > On Fri, Apr 12, 2019 at 08:43:32AM +0530, Srinath Mannam wrote: > > > > > Few SOCs have limitation that their PCIe host can't allow few inbound > > > > > address ranges. Allowed inbound address ranges are listed in > > > > > dma-ranges > > > > > DT property and this address ranges are required to do IOVA mapping. > > > > > Remaining address ranges have to be reserved in IOVA mapping. > > You probably want to fix the english in the first sentence. > The sense is reversed. > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 > 1PT, UK > Registration No: 1397386 (Wales) ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 2/3] iommu/dma: Reserve IOVA for PCIe inaccessible DMA address
On 02/05/2019 14:06, Lorenzo Pieralisi wrote:
On Thu, May 02, 2019 at 12:27:02PM +0100, Robin Murphy wrote:
Hi Lorenzo,
On 02/05/2019 12:01, Lorenzo Pieralisi wrote:
On Wed, May 01, 2019 at 11:06:25PM +0530, Srinath Mannam wrote:
dma_ranges field of PCI host bridge structure has resource entries in
sorted order of address range given through dma-ranges DT property. This
list is the accessible DMA address range. So that this resource list will
be processed and reserve IOVA address to the inaccessible address holes in
the list.
This method is similar to PCI IO resources address ranges reserving in
IOMMU for each EP connected to host bridge.
Signed-off-by: Srinath Mannam
Based-on-patch-by: Oza Pawandeep
Reviewed-by: Oza Pawandeep
Acked-by: Robin Murphy
---
drivers/iommu/dma-iommu.c | 19 +++
1 file changed, 19 insertions(+)
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 77aabe6..da94844 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -212,6 +212,7 @@ static void iova_reserve_pci_windows(struct pci_dev *dev,
struct pci_host_bridge *bridge = pci_find_host_bridge(dev->bus);
struct resource_entry *window;
unsigned long lo, hi;
+ phys_addr_t start = 0, end;
resource_list_for_each_entry(window, >windows) {
if (resource_type(window->res) != IORESOURCE_MEM)
@@ -221,6 +222,24 @@ static void iova_reserve_pci_windows(struct pci_dev *dev,
hi = iova_pfn(iovad, window->res->end - window->offset);
reserve_iova(iovad, lo, hi);
}
+
+ /* Get reserved DMA windows from host bridge */
+ resource_list_for_each_entry(window, >dma_ranges) {
If this list is not sorted it seems to me the logic in this loop is
broken and you can't rely on callers to sort it because it is not a
written requirement and it is not enforced (you know because you
wrote the code but any other developer is not supposed to guess
it).
Can't we rewrite this loop so that it does not rely on list
entries order ?
The original idea was that callers should be required to provide a sorted
list, since it keeps things nice and simple...
I understand, if it was self-contained in driver code that would be fine
but in core code with possible multiple consumers this must be
documented/enforced, somehow.
I won't merge this series unless you sort it, no pun intended.
Lorenzo
+ end = window->res->start - window->offset;
...so would you consider it sufficient to add
if (end < start)
dev_err(...);
We should also revert any IOVA reservation we did prior to this
error, right ?
I think it would be enough to propagate an error code back out through
iommu_dma_init_domain(), which should then end up aborting the whole
IOMMU setup - reserve_iova() isn't really designed to be undoable, but
since this is the kind of error that should only ever be hit during
driver or DT development, as long as we continue booting such that the
developer can clearly see what's gone wrong, I don't think we need
bother spending too much effort tidying up inside the unused domain.
Anyway, I think it is best to ensure it *is* sorted.
here, plus commenting the definition of pci_host_bridge::dma_ranges
that it must be sorted in ascending order?
I don't think that commenting dma_ranges would help much, I am more
keen on making it work by construction.
[ I guess it might even make sense to factor out the parsing and list
construction from patch #3 into an of_pci core helper from the beginning, so
that there's even less chance of another driver reimplementing it
incorrectly in future. ]
This makes sense IMO and I would like to take this approach if you
don't mind.
Sure - at some point it would be nice to wire this up to
pci-host-generic for Juno as well (with a parallel version for ACPI
_DMA), so from that viewpoint, the more groundwork in place the better :)
Thanks,
Robin.
Either this or we move the whole IOVA reservation and dma-ranges
parsing into PCI IProc.
Failing that, although I do prefer the "simple by construction"
approach, I'd have no objection to just sticking a list_sort() call in
here instead, if you'd rather it be entirely bulletproof.
I think what you outline above is a sensible way forward - if we
miss the merge window so be it.
Thanks,
Lorenzo
Robin.
+resv_iova:
+ if (end - start) {
+ lo = iova_pfn(iovad, start);
+ hi = iova_pfn(iovad, end);
+ reserve_iova(iovad, lo, hi);
+ }
+ start = window->res->end - window->offset + 1;
+ /* If window is last entry */
+ if (window->node.next == >dma_ranges &&
+ end != ~(dma_addr_t)0) {
+ end = ~(dma_addr_t)0;
+ goto resv_iova;
+ }
+ }
}
static
Re: [PATCH v2 RFC/RFT 1/5] ARM: dma-mapping: Add fallback normal page allocations
On Tue, Apr 30, 2019 at 04:24:21PM +0100, Catalin Marinas wrote: > My reading of the arm32 __dma_alloc() is that if the conditions are > right for the CMA allocator (allows blocking) and there is a default CMA > area or a per-device one, the call ends up in cma_alloc() without any > fallback if such allocation fails. Whether this is on purpose, I'm not > entirely sure. There are a couple of arm32 SoCs which call > dma_declare_contiguous() or dma_contiguous_reserve_area() and a few DT > files describing a specific CMA range (e.g. arch/arm/boot/dts/sun5i.dtsi > with a comment that address must be kept in the lower 256MB). > > If ZONE_DMA is set up correctly so that cma_alloc() is (or can be made) > interchangeable with alloc_pages(GFP_DMA) from a device DMA capability > perspective , I think it should be fine to have such fallback. Indeed. I missed arm32 being different from everyone else, but we already addresses that in another thread. Sorry for misleading everyone. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: implement generic dma_map_ops for IOMMUs v4
Hi Catalin and Will, can you quickly look over the arm64 parts? I'd really like to still get this series in for this merge window as it would conflict with a lot of dma-mapping work for next merge window, and we also have the amd and possibly intel iommu conversions to use it waiting. On Tue, Apr 30, 2019 at 06:51:49AM -0400, Christoph Hellwig wrote: > Hi Robin, > > please take a look at this series, which implements a completely generic > set of dma_map_ops for IOMMU drivers. This is done by taking the > existing arm64 code, moving it to drivers/iommu and then massaging it > so that it can also work for architectures with DMA remapping. This > should help future ports to support IOMMUs more easily, and also allow > to remove various custom IOMMU dma_map_ops implementations, like Tom > was planning to for the AMD one. > > A git tree is also available at: > > git://git.infradead.org/users/hch/misc.git dma-iommu-ops.3 > > Gitweb: > > > http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/dma-iommu-ops.3 > > Changes since v3: > - fold the separate patch to refactor mmap bounds checking > - don't warn on not finding a vm_area > - improve a commit log > - refactor __dma_iommu_free a little differently > - remove a minor MSI map cleanup to avoid a conflict with the >"Split iommu_dma_map_msi_msg" series > > Changes since v2: > - address various review comments and include patches from Robin > > Changes since v1: > - only include other headers in dma-iommu.h if CONFIG_DMA_IOMMU is enabled > - keep using a scatterlist in iommu_dma_alloc > - split out mmap/sgtable fixes and move them early in the series > - updated a few commit logs > ___ > iommu mailing list > iommu@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu ---end quoted text--- ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 4/7] dma-direct: provide generic support for uncached kernel segments
On Thu, May 02, 2019 at 12:08:01AM +, Paul Burton wrote:
> > Can you test the stack with the two updated patches and ack them if
> > they are fine? That would allow getting at least the infrastructure
> > and mips in for this merge window.
>
> Did you send a v2 of this patch?
>
> If so it hasn't showed up in my inbox, nor on the linux-mips archive on
> lore.kernel.org.
I did earlier in this thread. Here it is again:
---
>From 247ca658ebeb7c8d04918747ec8a0da45c36bcb8 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig
Date: Sun, 28 Apr 2019 13:23:26 -0500
Subject: dma-direct: provide generic support for uncached kernel segments
A few architectures support uncached kernel segments. In that case we get
an uncached mapping for a given physica address by using an offset in the
uncached segement. Implement support for this scheme in the generic
dma-direct code instead of duplicating it in arch hooks.
Signed-off-by: Christoph Hellwig
---
arch/Kconfig| 8
include/linux/dma-noncoherent.h | 3 +++
kernel/dma/direct.c | 17 +++--
3 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/arch/Kconfig b/arch/Kconfig
index 3368786a..ea22a8c894ec 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -249,6 +249,14 @@ config ARCH_HAS_FORTIFY_SOURCE
config ARCH_HAS_SET_MEMORY
bool
+#
+# Select if arch has an uncached kernel segment and provides the
+# uncached_kernel_address / cached_kernel_address symbols to use it
+#
+config ARCH_HAS_UNCACHED_SEGMENT
+ select ARCH_HAS_DMA_PREP_COHERENT
+ bool
+
# Select if arch init_task must go in the __init_task_data section
config ARCH_TASK_STRUCT_ON_STACK
bool
diff --git a/include/linux/dma-noncoherent.h b/include/linux/dma-noncoherent.h
index 9741767e400f..7e0126a04e02 100644
--- a/include/linux/dma-noncoherent.h
+++ b/include/linux/dma-noncoherent.h
@@ -80,4 +80,7 @@ static inline void arch_dma_prep_coherent(struct page *page,
size_t size)
}
#endif /* CONFIG_ARCH_HAS_DMA_PREP_COHERENT */
+void *uncached_kernel_address(void *addr);
+void *cached_kernel_address(void *addr);
+
#endif /* _LINUX_DMA_NONCOHERENT_H */
diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
index 2c2772e9702a..6688e1cee7d1 100644
--- a/kernel/dma/direct.c
+++ b/kernel/dma/direct.c
@@ -171,6 +171,13 @@ void *dma_direct_alloc_pages(struct device *dev, size_t
size,
*dma_handle = phys_to_dma(dev, page_to_phys(page));
}
memset(ret, 0, size);
+
+ if (IS_ENABLED(CONFIG_ARCH_HAS_UNCACHED_SEGMENT) &&
+ !dev_is_dma_coherent(dev) && !(attrs & DMA_ATTR_NON_CONSISTENT)) {
+ arch_dma_prep_coherent(page, size);
+ ret = uncached_kernel_address(ret);
+ }
+
return ret;
}
@@ -189,13 +196,18 @@ void dma_direct_free_pages(struct device *dev, size_t
size, void *cpu_addr,
if (force_dma_unencrypted())
set_memory_encrypted((unsigned long)cpu_addr, 1 << page_order);
+
+ if (IS_ENABLED(CONFIG_ARCH_HAS_UNCACHED_SEGMENT) &&
+ !dev_is_dma_coherent(dev) && !(attrs & DMA_ATTR_NON_CONSISTENT))
+ cpu_addr = cached_kernel_address(cpu_addr);
__dma_direct_free_pages(dev, size, virt_to_page(cpu_addr));
}
void *dma_direct_alloc(struct device *dev, size_t size,
dma_addr_t *dma_handle, gfp_t gfp, unsigned long attrs)
{
- if (!dev_is_dma_coherent(dev))
+ if (!IS_ENABLED(CONFIG_ARCH_HAS_UNCACHED_SEGMENT) &&
+ !dev_is_dma_coherent(dev))
return arch_dma_alloc(dev, size, dma_handle, gfp, attrs);
return dma_direct_alloc_pages(dev, size, dma_handle, gfp, attrs);
}
@@ -203,7 +215,8 @@ void *dma_direct_alloc(struct device *dev, size_t size,
void dma_direct_free(struct device *dev, size_t size,
void *cpu_addr, dma_addr_t dma_addr, unsigned long attrs)
{
- if (!dev_is_dma_coherent(dev))
+ if (!IS_ENABLED(CONFIG_ARCH_HAS_UNCACHED_SEGMENT) &&
+ !dev_is_dma_coherent(dev))
arch_dma_free(dev, size, cpu_addr, dma_addr, attrs);
else
dma_direct_free_pages(dev, size, cpu_addr, dma_addr, attrs);
--
2.20.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 2/3] iommu/dma: Reserve IOVA for PCIe inaccessible DMA address
On Thu, May 02, 2019 at 12:27:02PM +0100, Robin Murphy wrote:
> Hi Lorenzo,
>
> On 02/05/2019 12:01, Lorenzo Pieralisi wrote:
> > On Wed, May 01, 2019 at 11:06:25PM +0530, Srinath Mannam wrote:
> > > dma_ranges field of PCI host bridge structure has resource entries in
> > > sorted order of address range given through dma-ranges DT property. This
> > > list is the accessible DMA address range. So that this resource list will
> > > be processed and reserve IOVA address to the inaccessible address holes in
> > > the list.
> > >
> > > This method is similar to PCI IO resources address ranges reserving in
> > > IOMMU for each EP connected to host bridge.
> > >
> > > Signed-off-by: Srinath Mannam
> > > Based-on-patch-by: Oza Pawandeep
> > > Reviewed-by: Oza Pawandeep
> > > Acked-by: Robin Murphy
> > > ---
> > > drivers/iommu/dma-iommu.c | 19 +++
> > > 1 file changed, 19 insertions(+)
> > >
> > > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> > > index 77aabe6..da94844 100644
> > > --- a/drivers/iommu/dma-iommu.c
> > > +++ b/drivers/iommu/dma-iommu.c
> > > @@ -212,6 +212,7 @@ static void iova_reserve_pci_windows(struct pci_dev
> > > *dev,
> > > struct pci_host_bridge *bridge = pci_find_host_bridge(dev->bus);
> > > struct resource_entry *window;
> > > unsigned long lo, hi;
> > > + phys_addr_t start = 0, end;
> > > resource_list_for_each_entry(window, >windows) {
> > > if (resource_type(window->res) != IORESOURCE_MEM)
> > > @@ -221,6 +222,24 @@ static void iova_reserve_pci_windows(struct pci_dev
> > > *dev,
> > > hi = iova_pfn(iovad, window->res->end - window->offset);
> > > reserve_iova(iovad, lo, hi);
> > > }
> > > +
> > > + /* Get reserved DMA windows from host bridge */
> > > + resource_list_for_each_entry(window, >dma_ranges) {
> >
> > If this list is not sorted it seems to me the logic in this loop is
> > broken and you can't rely on callers to sort it because it is not a
> > written requirement and it is not enforced (you know because you
> > wrote the code but any other developer is not supposed to guess
> > it).
> >
> > Can't we rewrite this loop so that it does not rely on list
> > entries order ?
>
> The original idea was that callers should be required to provide a sorted
> list, since it keeps things nice and simple...
I understand, if it was self-contained in driver code that would be fine
but in core code with possible multiple consumers this must be
documented/enforced, somehow.
> > I won't merge this series unless you sort it, no pun intended.
> >
> > Lorenzo
> >
> > > + end = window->res->start - window->offset;
>
> ...so would you consider it sufficient to add
>
> if (end < start)
> dev_err(...);
We should also revert any IOVA reservation we did prior to this
error, right ?
Anyway, I think it is best to ensure it *is* sorted.
> here, plus commenting the definition of pci_host_bridge::dma_ranges
> that it must be sorted in ascending order?
I don't think that commenting dma_ranges would help much, I am more
keen on making it work by construction.
> [ I guess it might even make sense to factor out the parsing and list
> construction from patch #3 into an of_pci core helper from the beginning, so
> that there's even less chance of another driver reimplementing it
> incorrectly in future. ]
This makes sense IMO and I would like to take this approach if you
don't mind.
Either this or we move the whole IOVA reservation and dma-ranges
parsing into PCI IProc.
> Failing that, although I do prefer the "simple by construction"
> approach, I'd have no objection to just sticking a list_sort() call in
> here instead, if you'd rather it be entirely bulletproof.
I think what you outline above is a sensible way forward - if we
miss the merge window so be it.
Thanks,
Lorenzo
> Robin.
>
> > > +resv_iova:
> > > + if (end - start) {
> > > + lo = iova_pfn(iovad, start);
> > > + hi = iova_pfn(iovad, end);
> > > + reserve_iova(iovad, lo, hi);
> > > + }
> > > + start = window->res->end - window->offset + 1;
> > > + /* If window is last entry */
> > > + if (window->node.next == >dma_ranges &&
> > > + end != ~(dma_addr_t)0) {
> > > + end = ~(dma_addr_t)0;
> > > + goto resv_iova;
> > > + }
> > > + }
> > > }
> > > static int iova_reserve_iommu_regions(struct device *dev,
> > > --
> > > 2.7.4
> > >
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 9/9] dpaa_eth: fix SG frame cleanup
On Thu, 2019-05-02 at 12:58 +, Laurentiu Tudor wrote:
>
> > -Original Message-
> > From: Joakim Tjernlund
> > Sent: Thursday, May 2, 2019 1:37 PM
> >
> > On Thu, 2019-05-02 at 09:05 +, Laurentiu Tudor wrote:
> > > Hi Joakim,
> > >
> > > > -Original Message-
> > > > From: Joakim Tjernlund
> > > > Sent: Saturday, April 27, 2019 8:11 PM
> > > >
> > > > On Sat, 2019-04-27 at 10:10 +0300, laurentiu.tu...@nxp.com wrote:
> > > > > From: Laurentiu Tudor
> > > > >
> > > > > Fix issue with the entry indexing in the sg frame cleanup code being
> > > > > off-by-1. This problem showed up when doing some basic iperf tests
> > and
> > > > > manifested in traffic coming to a halt.
> > > > >
> > > > > Signed-off-by: Laurentiu Tudor
> > > > > Acked-by: Madalin Bucur
> > > >
> > > > Wasn't this a stable candidate too?
> > >
> > > Yes, it is. I forgot to add the cc:stable tag, sorry about that.
> >
> > Then this is a bug fix that should go directly to linus/stable.
> >
> > I note that
> > https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fstable%2Flinux.git%2Flog%2Fdrivers%2Fnet%2Fethernet%2Ffreescale%2Fdpaa%3Fh%3Dlinux-4.19.ydata=02%7C01%7CJoakim.Tjernlund%40infinera.com%7Cb88ecc951de649e5a55808d6cefdd286%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C636923986895133037sdata=ueUWI1%2BmNBHtlCoY9%2B1FreOUM8bHGiTYWhISy5nRoJk%3Dreserved=0
>
> Not sure I understand ... I don't see the patch in the link.
Sorry, I copied the wrong link:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/net/ethernet/freescale/dpaa?h=linux-4.19.y=0aafea5d4b22fe9403e89d82e02597e4493d5d0f
>
> > is in 4.19 but not in 4.14 , is it not appropriate for 4.14?
>
> I think it makes sense to go in both stable trees.
>
> ---
> Best Regards, Laurentiu
>
> > > > > ---
> > > > > drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 2 +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > > b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > > > index daede7272768..40420edc9ce6 100644
> > > > > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > > > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > > > @@ -1663,7 +1663,7 @@ static struct sk_buff
> > *dpaa_cleanup_tx_fd(const
> > > > struct dpaa_priv *priv,
> > > > > qm_sg_entry_get_len([0]),
> > dma_dir);
> > > > > /* remaining pages were mapped with
> > skb_frag_dma_map()
> > > > */
> > > > > - for (i = 1; i < nr_frags; i++) {
> > > > > + for (i = 1; i <= nr_frags; i++) {
> > > > > WARN_ON(qm_sg_entry_is_ext([i]));
> > > > >
> > > > > dma_unmap_page(dev, qm_sg_addr([i]),
> > > > > --
> > > > > 2.17.1
> > > > >
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v2 9/9] dpaa_eth: fix SG frame cleanup
> -Original Message-
> From: Joakim Tjernlund
> Sent: Thursday, May 2, 2019 1:37 PM
>
> On Thu, 2019-05-02 at 09:05 +, Laurentiu Tudor wrote:
> > Hi Joakim,
> >
> > > -Original Message-
> > > From: Joakim Tjernlund
> > > Sent: Saturday, April 27, 2019 8:11 PM
> > >
> > > On Sat, 2019-04-27 at 10:10 +0300, laurentiu.tu...@nxp.com wrote:
> > > > From: Laurentiu Tudor
> > > >
> > > > Fix issue with the entry indexing in the sg frame cleanup code being
> > > > off-by-1. This problem showed up when doing some basic iperf tests
> and
> > > > manifested in traffic coming to a halt.
> > > >
> > > > Signed-off-by: Laurentiu Tudor
> > > > Acked-by: Madalin Bucur
> > >
> > > Wasn't this a stable candidate too?
> >
> > Yes, it is. I forgot to add the cc:stable tag, sorry about that.
>
> Then this is a bug fix that should go directly to linus/stable.
>
> I note that
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/log/drivers/net/ethernet/freescale/dpaa?h=linux-4.19.y
Not sure I understand ... I don't see the patch in the link.
> is in 4.19 but not in 4.14 , is it not appropriate for 4.14?
I think it makes sense to go in both stable trees.
---
Best Regards, Laurentiu
> >
> > > > ---
> > > > drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > > index daede7272768..40420edc9ce6 100644
> > > > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > > @@ -1663,7 +1663,7 @@ static struct sk_buff
> *dpaa_cleanup_tx_fd(const
> > > struct dpaa_priv *priv,
> > > > qm_sg_entry_get_len([0]),
> dma_dir);
> > > >
> > > > /* remaining pages were mapped with
> skb_frag_dma_map()
> > > */
> > > > - for (i = 1; i < nr_frags; i++) {
> > > > + for (i = 1; i <= nr_frags; i++) {
> > > > WARN_ON(qm_sg_entry_is_ext([i]));
> > > >
> > > > dma_unmap_page(dev, qm_sg_addr([i]),
> > > > --
> > > > 2.17.1
> > > >
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 2/3] iommu/dma: Reserve IOVA for PCIe inaccessible DMA address
Hi Lorenzo,
On 02/05/2019 12:01, Lorenzo Pieralisi wrote:
On Wed, May 01, 2019 at 11:06:25PM +0530, Srinath Mannam wrote:
dma_ranges field of PCI host bridge structure has resource entries in
sorted order of address range given through dma-ranges DT property. This
list is the accessible DMA address range. So that this resource list will
be processed and reserve IOVA address to the inaccessible address holes in
the list.
This method is similar to PCI IO resources address ranges reserving in
IOMMU for each EP connected to host bridge.
Signed-off-by: Srinath Mannam
Based-on-patch-by: Oza Pawandeep
Reviewed-by: Oza Pawandeep
Acked-by: Robin Murphy
---
drivers/iommu/dma-iommu.c | 19 +++
1 file changed, 19 insertions(+)
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 77aabe6..da94844 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -212,6 +212,7 @@ static void iova_reserve_pci_windows(struct pci_dev *dev,
struct pci_host_bridge *bridge = pci_find_host_bridge(dev->bus);
struct resource_entry *window;
unsigned long lo, hi;
+ phys_addr_t start = 0, end;
resource_list_for_each_entry(window, >windows) {
if (resource_type(window->res) != IORESOURCE_MEM)
@@ -221,6 +222,24 @@ static void iova_reserve_pci_windows(struct pci_dev *dev,
hi = iova_pfn(iovad, window->res->end - window->offset);
reserve_iova(iovad, lo, hi);
}
+
+ /* Get reserved DMA windows from host bridge */
+ resource_list_for_each_entry(window, >dma_ranges) {
If this list is not sorted it seems to me the logic in this loop is
broken and you can't rely on callers to sort it because it is not a
written requirement and it is not enforced (you know because you
wrote the code but any other developer is not supposed to guess
it).
Can't we rewrite this loop so that it does not rely on list
entries order ?
The original idea was that callers should be required to provide a
sorted list, since it keeps things nice and simple...
I won't merge this series unless you sort it, no pun intended.
Lorenzo
+ end = window->res->start - window->offset;
...so would you consider it sufficient to add
if (end < start)
dev_err(...);
here, plus commenting the definition of pci_host_bridge::dma_ranges that
it must be sorted in ascending order?
[ I guess it might even make sense to factor out the parsing and list
construction from patch #3 into an of_pci core helper from the
beginning, so that there's even less chance of another driver
reimplementing it incorrectly in future. ]
Failing that, although I do prefer the "simple by construction"
approach, I'd have no objection to just sticking a list_sort() call in
here instead, if you'd rather it be entirely bulletproof.
Robin.
+resv_iova:
+ if (end - start) {
+ lo = iova_pfn(iovad, start);
+ hi = iova_pfn(iovad, end);
+ reserve_iova(iovad, lo, hi);
+ }
+ start = window->res->end - window->offset + 1;
+ /* If window is last entry */
+ if (window->node.next == >dma_ranges &&
+ end != ~(dma_addr_t)0) {
+ end = ~(dma_addr_t)0;
+ goto resv_iova;
+ }
+ }
}
static int iova_reserve_iommu_regions(struct device *dev,
--
2.7.4
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 2/3] iommu/dma: Reserve IOVA for PCIe inaccessible DMA address
On Wed, May 01, 2019 at 11:06:25PM +0530, Srinath Mannam wrote:
> dma_ranges field of PCI host bridge structure has resource entries in
> sorted order of address range given through dma-ranges DT property. This
> list is the accessible DMA address range. So that this resource list will
> be processed and reserve IOVA address to the inaccessible address holes in
> the list.
>
> This method is similar to PCI IO resources address ranges reserving in
> IOMMU for each EP connected to host bridge.
>
> Signed-off-by: Srinath Mannam
> Based-on-patch-by: Oza Pawandeep
> Reviewed-by: Oza Pawandeep
> Acked-by: Robin Murphy
> ---
> drivers/iommu/dma-iommu.c | 19 +++
> 1 file changed, 19 insertions(+)
>
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 77aabe6..da94844 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -212,6 +212,7 @@ static void iova_reserve_pci_windows(struct pci_dev *dev,
> struct pci_host_bridge *bridge = pci_find_host_bridge(dev->bus);
> struct resource_entry *window;
> unsigned long lo, hi;
> + phys_addr_t start = 0, end;
>
> resource_list_for_each_entry(window, >windows) {
> if (resource_type(window->res) != IORESOURCE_MEM)
> @@ -221,6 +222,24 @@ static void iova_reserve_pci_windows(struct pci_dev *dev,
> hi = iova_pfn(iovad, window->res->end - window->offset);
> reserve_iova(iovad, lo, hi);
> }
> +
> + /* Get reserved DMA windows from host bridge */
> + resource_list_for_each_entry(window, >dma_ranges) {
If this list is not sorted it seems to me the logic in this loop is
broken and you can't rely on callers to sort it because it is not a
written requirement and it is not enforced (you know because you
wrote the code but any other developer is not supposed to guess
it).
Can't we rewrite this loop so that it does not rely on list
entries order ?
I won't merge this series unless you sort it, no pun intended.
Lorenzo
> + end = window->res->start - window->offset;
> +resv_iova:
> + if (end - start) {
> + lo = iova_pfn(iovad, start);
> + hi = iova_pfn(iovad, end);
> + reserve_iova(iovad, lo, hi);
> + }
> + start = window->res->end - window->offset + 1;
> + /* If window is last entry */
> + if (window->node.next == >dma_ranges &&
> + end != ~(dma_addr_t)0) {
> + end = ~(dma_addr_t)0;
> + goto resv_iova;
> + }
> + }
> }
>
> static int iova_reserve_iommu_regions(struct device *dev,
> --
> 2.7.4
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v7 05/23] iommu: Introduce cache_invalidate API
On 02/05/2019 07:58, Auger Eric wrote:
> Hi Jean-Philippe,
>
> On 5/1/19 12:38 PM, Jean-Philippe Brucker wrote:
>> On 08/04/2019 13:18, Eric Auger wrote:
>>> +int iommu_cache_invalidate(struct iommu_domain *domain, struct device *dev,
>>> + struct iommu_cache_invalidate_info *inv_info)
>>> +{
>>> + int ret = 0;
>>> +
>>> + if (unlikely(!domain->ops->cache_invalidate))
>>> + return -ENODEV;
>>> +
>>> + ret = domain->ops->cache_invalidate(domain, dev, inv_info);
>>> +
>>> + return ret;
>>
>> Nit: you don't really need ret
>>
>> The UAPI looks good to me, so
>>
>> Reviewed-by: Jean-Philippe Brucker
> Just to make sure, do you accept changes proposed by Jacob in
> https://lkml.org/lkml/2019/4/29/659 ie.
> - the addition of NR_IOMMU_INVAL_GRANU in enum iommu_inv_granularity and
> - the addition of NR_IOMMU_CACHE_TYPE
Ah sorry, I forgot about that, I'll review the next version. Yes they
can be useful (maybe call them IOMMU_INV_GRANU_NR and
IOMMU_CACHE_INV_TYPE_NR?). I guess it's legal to export in UAPI values
that will change over time, as VFIO also does it in its enums.
Thanks,
Jean
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 9/9] dpaa_eth: fix SG frame cleanup
On Thu, 2019-05-02 at 09:05 +, Laurentiu Tudor wrote:
> Hi Joakim,
>
> > -Original Message-
> > From: Joakim Tjernlund
> > Sent: Saturday, April 27, 2019 8:11 PM
> >
> > On Sat, 2019-04-27 at 10:10 +0300, laurentiu.tu...@nxp.com wrote:
> > > From: Laurentiu Tudor
> > >
> > > Fix issue with the entry indexing in the sg frame cleanup code being
> > > off-by-1. This problem showed up when doing some basic iperf tests and
> > > manifested in traffic coming to a halt.
> > >
> > > Signed-off-by: Laurentiu Tudor
> > > Acked-by: Madalin Bucur
> >
> > Wasn't this a stable candidate too?
>
> Yes, it is. I forgot to add the cc:stable tag, sorry about that.
Then this is a bug fix that should go directly to linus/stable.
I note that
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/log/drivers/net/ethernet/freescale/dpaa?h=linux-4.19.y
is in 4.19 but not in 4.14 , is it not appropriate for 4.14?
Jocke
>
> ---
> Best Regards, Laurentiu
>
> > > ---
> > > drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > index daede7272768..40420edc9ce6 100644
> > > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > @@ -1663,7 +1663,7 @@ static struct sk_buff *dpaa_cleanup_tx_fd(const
> > struct dpaa_priv *priv,
> > > qm_sg_entry_get_len([0]), dma_dir);
> > >
> > > /* remaining pages were mapped with skb_frag_dma_map()
> > */
> > > - for (i = 1; i < nr_frags; i++) {
> > > + for (i = 1; i <= nr_frags; i++) {
> > > WARN_ON(qm_sg_entry_is_ext([i]));
> > >
> > > dma_unmap_page(dev, qm_sg_addr([i]),
> > > --
> > > 2.17.1
> > >
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v4 0/3] PCIe Host request to reserve IOVA
From: Srinath Mannam > Sent: 01 May 2019 16:23 ... > > > On Fri, Apr 12, 2019 at 08:43:32AM +0530, Srinath Mannam wrote: > > > > Few SOCs have limitation that their PCIe host can't allow few inbound > > > > address ranges. Allowed inbound address ranges are listed in dma-ranges > > > > DT property and this address ranges are required to do IOVA mapping. > > > > Remaining address ranges have to be reserved in IOVA mapping. You probably want to fix the english in the first sentence. The sense is reversed. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v2 7/9] dpaa_eth: fix iova handling for contiguous frames
> -Original Message- > From: Christoph Hellwig > Sent: Saturday, April 27, 2019 7:46 PM > > On Sat, Apr 27, 2019 at 10:10:29AM +0300, laurentiu.tu...@nxp.com wrote: > > From: Laurentiu Tudor > > > > The driver relies on the no longer valid assumption that dma addresses > > (iovas) are identical to physical addressees and uses phys_to_virt() to > > make iova -> vaddr conversions. Fix this by adding a function that does > > proper iova -> phys conversions using the iommu api and update the code > > to use it. > > Also, a dma_unmap_single() call had to be moved further down the code > > because iova -> vaddr conversions were required before the unmap. > > For now only the contiguous frame case is handled and the SG case is > > split in a following patch. > > While at it, clean-up a redundant dpaa_bpid2pool() and pass the bp > > as parameter. > > Err, this is broken. A driver using the DMA API has no business > call IOMMU APIs. Just save the _virtual_ address used for the mapping > away and use that again. We should not go through crazy gymnastics > like this. I think that due to the particularity of this hardware we don't have a way of saving the VA, but I'd let my colleagues maintaining this driver to comment more on why we need to do this. --- Best Regards, Laurentiu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v2 9/9] dpaa_eth: fix SG frame cleanup
Hi Joakim,
> -Original Message-
> From: Joakim Tjernlund
> Sent: Saturday, April 27, 2019 8:11 PM
>
> On Sat, 2019-04-27 at 10:10 +0300, laurentiu.tu...@nxp.com wrote:
> > From: Laurentiu Tudor
> >
> > Fix issue with the entry indexing in the sg frame cleanup code being
> > off-by-1. This problem showed up when doing some basic iperf tests and
> > manifested in traffic coming to a halt.
> >
> > Signed-off-by: Laurentiu Tudor
> > Acked-by: Madalin Bucur
>
> Wasn't this a stable candidate too?
Yes, it is. I forgot to add the cc:stable tag, sorry about that.
---
Best Regards, Laurentiu
> > ---
> > drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > index daede7272768..40420edc9ce6 100644
> > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > @@ -1663,7 +1663,7 @@ static struct sk_buff *dpaa_cleanup_tx_fd(const
> struct dpaa_priv *priv,
> > qm_sg_entry_get_len([0]), dma_dir);
> >
> > /* remaining pages were mapped with skb_frag_dma_map()
> */
> > - for (i = 1; i < nr_frags; i++) {
> > + for (i = 1; i <= nr_frags; i++) {
> > WARN_ON(qm_sg_entry_is_ext([i]));
> >
> > dma_unmap_page(dev, qm_sg_addr([i]),
> > --
> > 2.17.1
> >
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v7 05/23] iommu: Introduce cache_invalidate API
Hi Jean-Philippe,
On 5/1/19 12:38 PM, Jean-Philippe Brucker wrote:
> On 08/04/2019 13:18, Eric Auger wrote:
>> +int iommu_cache_invalidate(struct iommu_domain *domain, struct device *dev,
>> + struct iommu_cache_invalidate_info *inv_info)
>> +{
>> +int ret = 0;
>> +
>> +if (unlikely(!domain->ops->cache_invalidate))
>> +return -ENODEV;
>> +
>> +ret = domain->ops->cache_invalidate(domain, dev, inv_info);
>> +
>> +return ret;
>
> Nit: you don't really need ret
>
> The UAPI looks good to me, so
>
> Reviewed-by: Jean-Philippe Brucker
Just to make sure, do you accept changes proposed by Jacob in
https://lkml.org/lkml/2019/4/29/659 ie.
- the addition of NR_IOMMU_INVAL_GRANU in enum iommu_inv_granularity and
- the addition of NR_IOMMU_CACHE_TYPE
Thanks
Eric
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
