from:"Robin Murphy"

Re: [PATCH 10/10] ACPI: IORT: Allow COMPILE_TEST of IORT

2023-11-30 Thread Robin Murphy


On 29/11/2023 12:48 am, Jason Gunthorpe wrote:

The arm-smmu driver can COMPILE_TEST on x86, so expand this to also
enable the IORT code so it can be COMPILE_TEST'd too.

Signed-off-by: Jason Gunthorpe 
---
  drivers/acpi/Kconfig| 2 --
  drivers/acpi/Makefile   | 2 +-
  drivers/acpi/arm64/Kconfig  | 1 +
  drivers/acpi/arm64/Makefile | 2 +-
  drivers/iommu/Kconfig   | 1 +
  5 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index f819e760ff195a..3b7f77b227d13a 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -541,9 +541,7 @@ config ACPI_PFRUT
  To compile the drivers as modules, choose M here:
  the modules will be called pfr_update and pfr_telemetry.
  
-if ARM64

  source "drivers/acpi/arm64/Kconfig"
-endif
  
  config ACPI_PPTT

bool
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index eaa09bf52f1760..4e77ae37b80726 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -127,7 +127,7 @@ obj-y   += pmic/
  video-objs+= acpi_video.o video_detect.o
  obj-y += dptf/
  
-obj-$(CONFIG_ARM64)		+= arm64/

+obj-y  += arm64/
  
  obj-$(CONFIG_ACPI_VIOT)		+= viot.o
  
diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig

index b3ed6212244c1e..537d49d8ace69e 100644
--- a/drivers/acpi/arm64/Kconfig
+++ b/drivers/acpi/arm64/Kconfig
@@ -11,6 +11,7 @@ config ACPI_GTDT
  
  config ACPI_AGDI

bool "Arm Generic Diagnostic Dump and Reset Device Interface"
+   depends on ARM64
depends on ARM_SDE_INTERFACE
help
  Arm Generic Diagnostic Dump and Reset Device Interface (AGDI) is
diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile
index 143debc1ba4a9d..71d0e635599390 100644
--- a/drivers/acpi/arm64/Makefile
+++ b/drivers/acpi/arm64/Makefile
@@ -4,4 +4,4 @@ obj-$(CONFIG_ACPI_IORT) += iort.o
  obj-$(CONFIG_ACPI_GTDT)   += gtdt.o
  obj-$(CONFIG_ACPI_APMT)   += apmt.o
  obj-$(CONFIG_ARM_AMBA)+= amba.o
-obj-y  += dma.o init.o
+obj-$(CONFIG_ARM64)+= dma.o init.o
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 7673bb82945b6c..309378e76a9bc9 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -318,6 +318,7 @@ config ARM_SMMU
select IOMMU_API
select IOMMU_IO_PGTABLE_LPAE
select ARM_DMA_USE_IOMMU if ARM
+   select ACPI_IORT if ACPI


This is incomplete. If you want the driver to be responsible for 
enabling its own probing mechanisms then you need to select OF and ACPI 
too. And all the other drivers which probe from IORT should surely also 
select ACPI_IORT, and thus ACPI as well. And maybe the PCI core should 
as well because there are general properties of PCI host bridges and 
devices described in there?


But of course that's clearly backwards nonsense, because drivers do not 
and should not do that, so this change is not appropriate either. The 
IORT code may not be *functionally* arm64-specific, but logically it 
very much is - it serves a specification which is tied to the Arm 
architecture and describes Arm-architecture-specific concepts, within 
the wider context of ACPI on Arm itself only supporting AArch64, and not 
AArch32. It's also not like it's driver code that someone might use as 
an example and copy to a similar driver which could then run on 
different architectures where a latent theoretical bug becomes real. 
There's really no practical value to be had from compile-testing IORT.


Thanks,
Robin.

Re: [Nouveau] [PATCH 06/10] iommu: Replace iommu_device_lock with iommu_probe_device_lock

2023-11-29 Thread Robin Murphy


On 29/11/2023 12:48 am, Jason Gunthorpe wrote:

The iommu_device_lock protects the iommu_device_list which is only read by
iommu_ops_from_fwnode().

This is now always called under the iommu_probe_device_lock, so we don't
need to double lock the linked list. Use the iommu_probe_device_lock on
the write side too.


Please no, iommu_probe_device_lock() is a hack and we need to remove the 
*reason* it exists at all. And IMO just because iommu_present() is 
deprecated doesn't justify making it look utterly nonsensical - in no 
way does that have any relationship with probe_device, much less need to 
serialise against it!


Thanks,
Robin.


Signed-off-by: Jason Gunthorpe 
---
  drivers/iommu/iommu.c | 30 +-
  1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 08f29a1dfcd5f8..9557c2ec08d915 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -146,7 +146,6 @@ struct iommu_group_attribute iommu_group_attr_##_name = 
\
container_of(_kobj, struct iommu_group, kobj)
  
  static LIST_HEAD(iommu_device_list);

-static DEFINE_SPINLOCK(iommu_device_lock);
  
  static const struct bus_type * const iommu_buses[] = {

&platform_bus_type,
@@ -262,9 +261,9 @@ int iommu_device_register(struct iommu_device *iommu,
if (hwdev)
iommu->fwnode = dev_fwnode(hwdev);
  
-	spin_lock(&iommu_device_lock);

+   mutex_lock(&iommu_probe_device_lock);
list_add_tail(&iommu->list, &iommu_device_list);
-   spin_unlock(&iommu_device_lock);
+   mutex_unlock(&iommu_probe_device_lock);
  
  	for (int i = 0; i < ARRAY_SIZE(iommu_buses) && !err; i++)

err = bus_iommu_probe(iommu_buses[i]);
@@ -279,9 +278,9 @@ void iommu_device_unregister(struct iommu_device *iommu)
for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++)
bus_for_each_dev(iommu_buses[i], NULL, iommu, 
remove_iommu_group);
  
-	spin_lock(&iommu_device_lock);

+   mutex_lock(&iommu_probe_device_lock);
list_del(&iommu->list);
-   spin_unlock(&iommu_device_lock);
+   mutex_unlock(&iommu_probe_device_lock);
  
  	/* Pairs with the alloc in generic_single_device_group() */

iommu_group_put(iommu->singleton_group);
@@ -316,9 +315,9 @@ int iommu_device_register_bus(struct iommu_device *iommu,
if (err)
return err;
  
-	spin_lock(&iommu_device_lock);

+   mutex_lock(&iommu_probe_device_lock);
list_add_tail(&iommu->list, &iommu_device_list);
-   spin_unlock(&iommu_device_lock);
+   mutex_unlock(&iommu_probe_device_lock);
  
  	err = bus_iommu_probe(bus);

if (err) {
@@ -2033,9 +2032,9 @@ bool iommu_present(const struct bus_type *bus)
  
  	for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) {

if (iommu_buses[i] == bus) {
-   spin_lock(&iommu_device_lock);
+   mutex_lock(&iommu_probe_device_lock);
ret = !list_empty(&iommu_device_list);
-   spin_unlock(&iommu_device_lock);
+   mutex_unlock(&iommu_probe_device_lock);
}
}
return ret;
@@ -2980,17 +2979,14 @@ EXPORT_SYMBOL_GPL(iommu_default_passthrough);
  
  const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)

  {
-   const struct iommu_ops *ops = NULL;
struct iommu_device *iommu;
  
-	spin_lock(&iommu_device_lock);

+   lockdep_assert_held(&iommu_probe_device_lock);
+
list_for_each_entry(iommu, &iommu_device_list, list)
-   if (iommu->fwnode == fwnode) {
-   ops = iommu->ops;
-   break;
-   }
-   spin_unlock(&iommu_device_lock);
-   return ops;
+   if (iommu->fwnode == fwnode)
+   return iommu->ops;
+   return NULL;
  }
  
  int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,

Re: [Nouveau] [PATCH v2 04/10] iommu/dma: Use the gfp parameter in __iommu_dma_alloc_noncontiguous()

2023-01-20 Thread Robin Murphy


On 2023-01-18 18:00, Jason Gunthorpe wrote:

Change the sg_alloc_table_from_pages() allocation that was hardwired to
GFP_KERNEL to use the gfp parameter like the other allocations in this
function.

Auditing says this is never called from an atomic context, so it is safe
as is, but reads wrong.


I think the point may have been that the sgtable metadata is a 
logically-distinct allocation from the buffer pages themselves. Much 
like the allocation of the pages array itself further down in 
__iommu_dma_alloc_pages(). I see these days it wouldn't be catastrophic 
to pass GFP_HIGHMEM into __get_free_page() via sg_kmalloc(), but still, 
allocating implementation-internal metadata with all the same 
constraints as a DMA buffer has just as much smell of wrong about it IMO.


I'd say the more confusing thing about this particular context is why 
we're using iommu_map_sg_atomic() further down - that seems to have been 
an oversight in 781ca2de89ba, since this particular path has never 
supported being called in atomic context.


Overall I'm starting to wonder if it might not be better to stick a "use 
GFP_KERNEL_ACCOUNT if you allocate" flag in the domain for any level of 
the API internals to pick up as appropriate, rather than propagate 
per-call gfp flags everywhere. As it stands we're still missing 
potential pagetable and other domain-related allocations by drivers in 
.attach_dev and even (in probably-shouldn't-really-happen cases) 
.unmap_pages...


Thanks,
Robin.


Signed-off-by: Jason Gunthorpe 
---
  drivers/iommu/dma-iommu.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 8c2788633c1766..e4bf1bb159f7c7 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -822,7 +822,7 @@ static struct page **__iommu_dma_alloc_noncontiguous(struct 
device *dev,
if (!iova)
goto out_free_pages;
  
-	if (sg_alloc_table_from_pages(sgt, pages, count, 0, size, GFP_KERNEL))

+   if (sg_alloc_table_from_pages(sgt, pages, count, 0, size, gfp))
goto out_free_iova;
  
  	if (!(ioprot & IOMMU_CACHE)) {

Re: [Nouveau] [PATCH 1/8] iommu: Add a gfp parameter to iommu_map()

2023-01-06 Thread Robin Murphy


On 2023-01-06 16:42, Jason Gunthorpe wrote:

The internal mechanisms support this, but instead of exposting the gfp to
the caller it wrappers it into iommu_map() and iommu_map_atomic()

Fix this instead of adding more variants for GFP_KERNEL_ACCOUNT.


FWIW, since we *do* have two variants already, I think I'd have a mild 
preference for leaving the regular map calls as-is (i.e. implicit 
GFP_KERNEL), and just generalising the _atomic versions for the special 
cases.


However, echoing the recent activity over on the DMA API side of things, 
I think it's still worth proactively constraining the set of permissible 
flags, lest we end up with more weird problems if stuff that doesn't 
really make sense, like GFP_COMP or zone flags, manages to leak through 
(that may have been part of the reason for having the current wrappers 
rather than a bare gfp argument in the first place, I forget now).


Thanks,
Robin.


Signed-off-by: Jason Gunthorpe 
---
  arch/arm/mm/dma-mapping.c   | 11 +++
  .../gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c |  3 ++-
  drivers/gpu/drm/tegra/drm.c |  2 +-
  drivers/gpu/host1x/cdma.c   |  2 +-
  drivers/infiniband/hw/usnic/usnic_uiom.c|  4 ++--
  drivers/iommu/dma-iommu.c   |  2 +-
  drivers/iommu/iommu.c   | 17 ++---
  drivers/iommu/iommufd/pages.c   |  6 --
  drivers/media/platform/qcom/venus/firmware.c|  2 +-
  drivers/net/ipa/ipa_mem.c   |  6 --
  drivers/net/wireless/ath/ath10k/snoc.c  |  2 +-
  drivers/net/wireless/ath/ath11k/ahb.c   |  4 ++--
  drivers/remoteproc/remoteproc_core.c|  5 +++--
  drivers/vfio/vfio_iommu_type1.c |  9 +
  drivers/vhost/vdpa.c|  2 +-
  include/linux/iommu.h   |  4 ++--
  16 files changed, 43 insertions(+), 38 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index c135f6e37a00ca..8bc01071474ab7 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -984,7 +984,8 @@ __iommu_create_mapping(struct device *dev, struct page 
**pages, size_t size,
  
  		len = (j - i) << PAGE_SHIFT;

ret = iommu_map(mapping->domain, iova, phys, len,
-   __dma_info_to_prot(DMA_BIDIRECTIONAL, attrs));
+   __dma_info_to_prot(DMA_BIDIRECTIONAL, attrs),
+   GFP_KERNEL);
if (ret < 0)
goto fail;
iova += len;
@@ -1207,7 +1208,8 @@ static int __map_sg_chunk(struct device *dev, struct 
scatterlist *sg,
  
  		prot = __dma_info_to_prot(dir, attrs);
  
-		ret = iommu_map(mapping->domain, iova, phys, len, prot);

+   ret = iommu_map(mapping->domain, iova, phys, len, prot,
+   GFP_KERNEL);
if (ret < 0)
goto fail;
count += len >> PAGE_SHIFT;
@@ -1379,7 +1381,8 @@ static dma_addr_t arm_iommu_map_page(struct device *dev, 
struct page *page,
  
  	prot = __dma_info_to_prot(dir, attrs);
  
-	ret = iommu_map(mapping->domain, dma_addr, page_to_phys(page), len, prot);

+   ret = iommu_map(mapping->domain, dma_addr, page_to_phys(page), len,
+   prot, GFP_KERNEL);
if (ret < 0)
goto fail;
  
@@ -1443,7 +1446,7 @@ static dma_addr_t arm_iommu_map_resource(struct device *dev,
  
  	prot = __dma_info_to_prot(dir, attrs) | IOMMU_MMIO;
  
-	ret = iommu_map(mapping->domain, dma_addr, addr, len, prot);

+   ret = iommu_map(mapping->domain, dma_addr, addr, len, prot, GFP_KERNEL);
if (ret < 0)
goto fail;
  
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c

index 648ecf5a8fbc2a..a4ac94a2ab57fc 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c
@@ -475,7 +475,8 @@ gk20a_instobj_ctor_iommu(struct gk20a_instmem *imem, u32 
npages, u32 align,
u32 offset = (r->offset + i) << imem->iommu_pgshift;
  
  		ret = iommu_map(imem->domain, offset, node->dma_addrs[i],

-   PAGE_SIZE, IOMMU_READ | IOMMU_WRITE);
+   PAGE_SIZE, IOMMU_READ | IOMMU_WRITE,
+   GFP_KERNEL);
if (ret < 0) {
nvkm_error(subdev, "IOMMU mapping failure: %d\n", ret);
  
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c

index 7bd2e65c2a16c5..6ca9f396e55be4 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -1057,7 +1057,7 @@ void *tegra_drm_alloc(struct tegra_drm *tegra, size_t 
size, dma_addr_t *dma)
  
  	*dma = iova_dma_addr(&tegra->carveout.domain, alloc);

err = iomm

[Nouveau] [PATCH] drm/nouveau/tegra: Stop using iommu_present()

2022-04-05 Thread Robin Murphy

Even if some IOMMU has registered itself on the platform "bus", that
doesn't necessarily mean it provides translation for the device we
care about. Replace iommu_present() with a more appropriate check.

Signed-off-by: Robin Murphy 
---
 drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c 
b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
index 992cc285f2fe..2ed528c065fa 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
@@ -123,7 +123,7 @@ nvkm_device_tegra_probe_iommu(struct nvkm_device_tegra 
*tdev)
 
mutex_init(&tdev->iommu.mutex);
 
-   if (iommu_present(&platform_bus_type)) {
+   if (device_iommu_mapped(dev)) {
tdev->iommu.domain = iommu_domain_alloc(&platform_bus_type);
if (!tdev->iommu.domain)
goto error;
-- 
2.28.0.dirty

Re: [Nouveau] [PATCH 1/1] drm/nouveau/device: Get right pgsize_bitmap of iommu_domain

2021-12-20 Thread Robin Murphy


On 2021-12-18 07:45, Lu Baolu wrote:

The suported page sizes of an iommu_domain are saved in the pgsize_bitmap
field. Retrieve the value from the right place.

Fixes: 58fd9375c2c534 ("drm/nouveau/platform: probe IOMMU if present")


...except domain->pgsize_bitmap was introduced more than a year after 
that commit ;)


As an improvement rather than a fix, though,

Reviewed-by: Robin Murphy 


Signed-off-by: Lu Baolu 
---
  drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c 
b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
index d0d52c1d4aee..992cc285f2fe 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
@@ -133,7 +133,7 @@ nvkm_device_tegra_probe_iommu(struct nvkm_device_tegra 
*tdev)
 * or equal to the system's PAGE_SIZE, with a preference if
 * both are equal.
 */
-   pgsize_bitmap = tdev->iommu.domain->ops->pgsize_bitmap;
+   pgsize_bitmap = tdev->iommu.domain->pgsize_bitmap;
if (pgsize_bitmap & PAGE_SIZE) {
tdev->iommu.pgshift = PAGE_SHIFT;
} else {

Re: [Nouveau] [PATCH v5 16/16] of: Add plumbing for restricted DMA pool

2021-04-23 Thread Robin Murphy


On 2021-04-22 09:15, Claire Chang wrote:

If a device is not behind an IOMMU, we look up the device node and set
up the restricted DMA when the restricted-dma-pool is presented.

Signed-off-by: Claire Chang 
---
  drivers/of/address.c| 25 +
  drivers/of/device.c |  3 +++
  drivers/of/of_private.h |  5 +
  3 files changed, 33 insertions(+)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 54f221dde267..fff3adfe4986 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -8,6 +8,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -1109,6 +1110,30 @@ bool of_dma_is_coherent(struct device_node *np)
  }
  EXPORT_SYMBOL_GPL(of_dma_is_coherent);
  
+int of_dma_set_restricted_buffer(struct device *dev)

+{
+   struct device_node *node;
+   int count, i;
+
+   if (!dev->of_node)
+   return 0;
+
+   count = of_property_count_elems_of_size(dev->of_node, "memory-region",
+   sizeof(phandle));
+   for (i = 0; i < count; i++) {
+   node = of_parse_phandle(dev->of_node, "memory-region", i);
+   /* There might be multiple memory regions, but only one
+* restriced-dma-pool region is allowed.
+*/


What's the use-case for having multiple regions if the restricted pool 
is by definition the only one accessible?


Robin.


+   if (of_device_is_compatible(node, "restricted-dma-pool") &&
+   of_device_is_available(node))
+   return of_reserved_mem_device_init_by_idx(
+   dev, dev->of_node, i);
+   }
+
+   return 0;
+}
+
  /**
   * of_mmio_is_nonposted - Check if device uses non-posted MMIO
   * @np:   device node
diff --git a/drivers/of/device.c b/drivers/of/device.c
index c5a9473a5fb1..d8d865223e51 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -165,6 +165,9 @@ int of_dma_configure_id(struct device *dev, struct 
device_node *np,
  
  	arch_setup_dma_ops(dev, dma_start, size, iommu, coherent);
  
+	if (!iommu)

+   return of_dma_set_restricted_buffer(dev);
+
return 0;
  }
  EXPORT_SYMBOL_GPL(of_dma_configure_id);
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index d717efbd637d..e9237f5eff48 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -163,12 +163,17 @@ struct bus_dma_region;
  #if defined(CONFIG_OF_ADDRESS) && defined(CONFIG_HAS_DMA)
  int of_dma_get_range(struct device_node *np,
const struct bus_dma_region **map);
+int of_dma_set_restricted_buffer(struct device *dev);
  #else
  static inline int of_dma_get_range(struct device_node *np,
const struct bus_dma_region **map)
  {
return -ENODEV;
  }
+static inline int of_dma_get_restricted_buffer(struct device *dev)
+{
+   return -ENODEV;
+}
  #endif
  
  #endif /* _LINUX_OF_PRIVATE_H */



___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v5 14/16] dma-direct: Allocate memory from restricted DMA pool if available

2021-04-23 Thread Robin Murphy


On 2021-04-22 09:15, Claire Chang wrote:

The restricted DMA pool is preferred if available.

The restricted DMA pools provide 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 a way to lock down the memory access, e.g., MPU.

Signed-off-by: Claire Chang 
---
  kernel/dma/direct.c | 35 ++-
  1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
index 7a27f0510fcc..29523d2a9845 100644
--- a/kernel/dma/direct.c
+++ b/kernel/dma/direct.c
@@ -78,6 +78,10 @@ static bool dma_coherent_ok(struct device *dev, phys_addr_t 
phys, size_t size)
  static void __dma_direct_free_pages(struct device *dev, struct page *page,
size_t size)
  {
+#ifdef CONFIG_DMA_RESTRICTED_POOL
+   if (swiotlb_free(dev, page, size))
+   return;
+#endif
dma_free_contiguous(dev, page, size);
  }
  
@@ -92,7 +96,17 @@ static struct page *__dma_direct_alloc_pages(struct device *dev, size_t size,
  
  	gfp |= dma_direct_optimal_gfp_mask(dev, dev->coherent_dma_mask,

   &phys_limit);
-   page = dma_alloc_contiguous(dev, size, gfp);
+
+#ifdef CONFIG_DMA_RESTRICTED_POOL
+   page = swiotlb_alloc(dev, size);
+   if (page && !dma_coherent_ok(dev, page_to_phys(page), size)) {
+   __dma_direct_free_pages(dev, page, size);
+   page = NULL;
+   }
+#endif
+
+   if (!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);
page = NULL;
@@ -148,7 +162,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_dev_swiotlb_force(dev)) {
page = __dma_direct_alloc_pages(dev, size, gfp & ~__GFP_ZERO);
if (!page)
return NULL;
@@ -161,8 +175,8 @@ void *dma_direct_alloc(struct device *dev, size_t size,
}
  
  	if (!IS_ENABLED(CONFIG_ARCH_HAS_DMA_SET_UNCACHED) &&

-   !IS_ENABLED(CONFIG_DMA_DIRECT_REMAP) &&
-   !dev_is_dma_coherent(dev))
+   !IS_ENABLED(CONFIG_DMA_DIRECT_REMAP) && !dev_is_dma_coherent(dev) &&
+   !is_dev_swiotlb_force(dev))
return arch_dma_alloc(dev, size, dma_handle, gfp, attrs);
  
  	/*

@@ -172,7 +186,9 @@ void *dma_direct_alloc(struct device *dev, size_t size,
if (IS_ENABLED(CONFIG_DMA_COHERENT_POOL) &&
!gfpflags_allow_blocking(gfp) &&
(force_dma_unencrypted(dev) ||
-(IS_ENABLED(CONFIG_DMA_DIRECT_REMAP) && 
!dev_is_dma_coherent(dev
+(IS_ENABLED(CONFIG_DMA_DIRECT_REMAP) &&
+ !dev_is_dma_coherent(dev))) &&
+   !is_dev_swiotlb_force(dev))
return dma_direct_alloc_from_pool(dev, size, dma_handle, gfp);
  
  	/* we always manually zero the memory once we are done */

@@ -253,15 +269,15 @@ void dma_direct_free(struct device *dev, size_t size,
unsigned int page_order = get_order(size);
  
  	if ((attrs & DMA_ATTR_NO_KERNEL_MAPPING) &&

-   !force_dma_unencrypted(dev)) {
+   !force_dma_unencrypted(dev) && !is_dev_swiotlb_force(dev)) {
/* cpu_addr is a struct page cookie, not a kernel address */
dma_free_contiguous(dev, cpu_addr, size);
return;
}
  
  	if (!IS_ENABLED(CONFIG_ARCH_HAS_DMA_SET_UNCACHED) &&

-   !IS_ENABLED(CONFIG_DMA_DIRECT_REMAP) &&
-   !dev_is_dma_coherent(dev)) {
+   !IS_ENABLED(CONFIG_DMA_DIRECT_REMAP) && !dev_is_dma_coherent(dev) &&
+   !is_dev_swiotlb_force(dev)) {
arch_dma_free(dev, size, cpu_addr, dma_addr, attrs);
return;
}
@@ -289,7 +305,8 @@ struct page *dma_direct_alloc_pages(struct device *dev, 
size_t size,
void *ret;
  
  	if (IS_ENABLED(CONFIG_DMA_COHERENT_POOL) &&

-   force_dma_unencrypted(dev) && !gfpflags_allow_blocking(gfp))
+   force_dma_unencrypted(dev) && !gfpflags_allow_blocking(gfp) &&
+   !is_dev_swiotlb_force(dev))
return dma_direct_alloc_from_pool(dev, size, dma_handle, gfp);


Wait, this seems broken for non-coherent devices - in that case we need 
to return a non-cacheable address, but we can't simply fall through into 
the remapping path below in GFP_ATOMIC context. That's why we need the 
atomic pool concept in the first place :/


Unless I've overlooked something, we're still using the regular 
cacheable linear map address of the dma_io_tlb_mem buffer, no?


Robin.

  
  	page = __dma_direct_alloc_pages(dev, size,

Re: [Nouveau] [PATCH v5 08/16] swiotlb: Update is_swiotlb_active to add a struct device argument

2021-04-23 Thread Robin Murphy


On 2021-04-22 09:15, Claire Chang wrote:

Update is_swiotlb_active to add a struct device argument. This will be
useful later to allow for restricted DMA pool.

Signed-off-by: Claire Chang 
---
  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 ce6b664b10aa..7d48c433446b 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(NULL)) {
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 e8b506a6685b..2a2ae6d6cf6d 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(NULL);
  #endif
  
  	ret = ttm_device_init(&drm->ttm.bdev, &nouveau_bo_driver, drm->dev->dev,

diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
index b7a8f3a1921f..6d548ce53ce7 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(&pcifront_dev_lock);
  
-	if (!err && !is_swiotlb_active()) {

+   if (!err && !is_swiotlb_active(NULL)) {
err = pci_xen_swiotlb_init_late();
if (err)
dev_err(&pdev->xdev->dev, "Could not setup SWIOTLB!\n");
diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
index 2a6cca07540b..c530c976d18b 100644
--- a/include/linux/swiotlb.h
+++ b/include/linux/swiotlb.h
@@ -123,7 +123,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
@@ -143,7 +143,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))


I wonder if it's worth trying to fold these other conditions into 
is_swiotlb_active() itself? I'm not entirely sure what matters for Xen, 
but for the other cases it seems like they probably only care about 
whether bouncing may occur for their particular device or not (possibly 
they want to be using dma_max_mapping_size() now anyway - TBH I'm 
struggling to make sense of what the swiotlb_max_segment business is 
supposed to mean).


Otherwise, patch #9 will need to touch here as well to make sure that 
per-device forced bouncing is reflected correctly.


Robin.


return swiotlb_max_mapping_size(dev);
return SIZE_MAX;
diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index ffbb8724e06c..1d221343f1c8 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -659,9 +659,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 get_io_tlb_mem(dev) != NULL;
  }
  EXPORT_SYMBOL_GPL(is_swiotlb_active);
  


___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH RFC v1 2/6] swiotlb: convert variables to arrays

2021-02-04 Thread Robin Murphy


On 2021-02-04 07:29, Christoph Hellwig wrote:

On Wed, Feb 03, 2021 at 03:37:05PM -0800, Dongli Zhang wrote:

This patch converts several swiotlb related variables to arrays, in
order to maintain stat/status for different swiotlb buffers. Here are
variables involved:

- io_tlb_start and io_tlb_end
- io_tlb_nslabs and io_tlb_used
- io_tlb_list
- io_tlb_index
- max_segment
- io_tlb_orig_addr
- no_iotlb_memory

There is no functional change and this is to prepare to enable 64-bit
swiotlb.


Claire Chang (on Cc) already posted a patch like this a month ago,
which looks much better because it actually uses a struct instead
of all the random variables.


Indeed, I skimmed the cover letter and immediately thought that this 
whole thing is just the restricted DMA pool concept[1] again, only from 
a slightly different angle.


Robin.

[1] 
https://lore.kernel.org/linux-iommu/20210106034124.30560-1-tien...@chromium.org/

___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH 08/18] dma-mapping: add a new dma_alloc_noncoherent API

2020-09-25 Thread Robin Murphy


On 2020-09-15 16:51, Christoph Hellwig wrote:
[...]

+These APIs allow to allocate pages in the kernel direct mapping that are
+guaranteed to be DMA addressable.  This means that unlike dma_alloc_coherent,
+virt_to_page can be called on the resulting address, and the resulting


Nit: if we explicitly describe this as if it's a guarantee that can be 
relied upon...



+struct page can be used for everything a struct page is suitable for.


[...]

+This routine allocates a region of  bytes of consistent memory.  It
+returns a pointer to the allocated region (in the processor's virtual address
+space) or NULL if the allocation failed.  The returned memory may or may not
+be in the kernels direct mapping.  Drivers must not call virt_to_page on
+the returned memory region.


...then forbid this document's target audience from relying on it, 
something seems off. At the very least it's unhelpfully unclear :/


Given patch #17, I suspect that the first paragraph is the one that's no 
longer true.


Robin.
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [trivial PATCH] treewide: Convert switch/case fallthrough; to break;

2020-09-11 Thread Robin Murphy


On 2020-09-09 21:06, Joe Perches wrote:

fallthrough to a separate case/default label break; isn't very readable.

Convert pseudo-keyword fallthrough; statements to a simple break; when
the next label is case or default and the only statement in the next
label block is break;

Found using:

$ grep-2.5.4 -rP --include=*.[ch] -n 
"fallthrough;(\s*(case\s+\w+|default)\s*:\s*){1,7}break;" *

Miscellanea:

o Move or coalesce a couple label blocks above a default: block.

Signed-off-by: Joe Perches 
---

Compiled allyesconfig x86-64 only.
A few files for other arches were not compiled.



[...]

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c 
b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index c192544e874b..743db1abec40 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -3777,7 +3777,7 @@ static int arm_smmu_device_hw_probe(struct 
arm_smmu_device *smmu)
switch (FIELD_GET(IDR0_TTF, reg)) {
case IDR0_TTF_AARCH32_64:
smmu->ias = 40;
-   fallthrough;
+   break;
case IDR0_TTF_AARCH64:
break;
default:


I have to say I don't really agree with the readability argument for 
this one - a fallthrough is semantically correct here, since the first 
case is a superset of the second. It just happens that anything we would 
do for the common subset is implicitly assumed (there are other 
potential cases we simply haven't added support for at the moment), thus 
the second case is currently empty.


This change actively obfuscates that distinction.

Robin.
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH 05/28] media/v4l2: remove V4L2-FLAG-MEMORY-NON-CONSISTENT

2020-08-19 Thread Robin Murphy


On 2020-08-19 13:49, Tomasz Figa wrote:

On Wed, Aug 19, 2020 at 1:51 PM Robin Murphy  wrote:


Hi Tomasz,

On 2020-08-19 12:16, Tomasz Figa wrote:

Hi Christoph,

On Wed, Aug 19, 2020 at 8:56 AM Christoph Hellwig  wrote:


The V4L2-FLAG-MEMORY-NON-CONSISTENT flag is entirely unused,


Could you explain what makes you think it's unused? It's a feature of
the UAPI generally supported by the videobuf2 framework and relied on
by Chromium OS to get any kind of reasonable performance when
accessing V4L2 buffers in the userspace.


and causes
weird gymanstics with the DMA_ATTR_NON_CONSISTENT flag, which is
unimplemented except on PARISC and some MIPS configs, and about to be
removed.


It is implemented by the generic DMA mapping layer [1], which is used
by a number of architectures including ARM64 and supposed to be used
by new architectures going forward.


AFAICS all that V4L2_FLAG_MEMORY_NON_CONSISTENT does is end up
controling whether DMA_ATTR_NON_CONSISTENT is added to vb2_queue::dma_attrs.

Please can you point to where DMA_ATTR_NON_CONSISTENT does anything at
all on arm64?



With the default config it doesn't, but with
CONFIG_DMA_NONCOHERENT_CACHE_SYNC enabled it makes dma_pgprot() keep
the pgprot value as is, without enforcing coherence attributes.


How active are the PA-RISC and MIPS ports of Chromium OS?

Hacking CONFIG_DMA_NONCOHERENT_CACHE_SYNC into an architecture that 
doesn't provide dma_cache_sync() is wrong, since at worst it may break 
other drivers. If downstream is wildly misusing an API then so be it, 
but it's hardly a strong basis for an upstream argument.



Also, I posit that videobuf2 is not actually relying on
DMA_ATTR_NON_CONSISTENT anyway, since it's clearly not using it properly:

"By using this API, you are guaranteeing to the platform
that you have all the correct and necessary sync points for this memory
in the driver should it choose to return non-consistent memory."

$ git grep dma_cache_sync drivers/media
$


AFAIK dma_cache_sync() isn't the only way to perform the cache
synchronization. The earlier patch series that I reviewed relied on
dma_get_sgtable() and then dma_sync_sg_*() (which existed in the
vb2-dc since forever [1]). However, it looks like with the final code
the sgtable isn't acquired and the synchronization isn't happening, so
you have a point.


Using the streaming sync calls on coherent allocations has also always 
been wrong per the API, regardless of the bodies of code that have 
happened to get away with it for so long.



FWIW, I asked back in time what the plan is for non-coherent
allocations and it seemed like DMA_ATTR_NON_CONSISTENT and
dma_sync_*() was supposed to be the right thing to go with. [2] The
same thread also explains why dma_alloc_pages() isn't suitable for the
users of dma_alloc_attrs() and DMA_ATTR_NON_CONSISTENT.


AFAICS even back then Christoph was implying getting rid of 
NON_CONSISTENT and *replacing* it with something streaming-API-based - 
i.e. this series - not encouraging mixing the existing APIs. It doesn't 
seem impossible to implement a remapping version of this new 
dma_alloc_pages() for IOMMU-backed ops if it's really warranted 
(although at that point it seems like "non-coherent" vb2-dc starts to 
have significant conceptual overlap with vb2-sg).


Robin.
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH 05/28] media/v4l2: remove V4L2-FLAG-MEMORY-NON-CONSISTENT

2020-08-19 Thread Robin Murphy


Hi Tomasz,

On 2020-08-19 12:16, Tomasz Figa wrote:

Hi Christoph,

On Wed, Aug 19, 2020 at 8:56 AM Christoph Hellwig  wrote:


The V4L2-FLAG-MEMORY-NON-CONSISTENT flag is entirely unused,


Could you explain what makes you think it's unused? It's a feature of
the UAPI generally supported by the videobuf2 framework and relied on
by Chromium OS to get any kind of reasonable performance when
accessing V4L2 buffers in the userspace.


and causes
weird gymanstics with the DMA_ATTR_NON_CONSISTENT flag, which is
unimplemented except on PARISC and some MIPS configs, and about to be
removed.


It is implemented by the generic DMA mapping layer [1], which is used
by a number of architectures including ARM64 and supposed to be used
by new architectures going forward.


AFAICS all that V4L2_FLAG_MEMORY_NON_CONSISTENT does is end up 
controling whether DMA_ATTR_NON_CONSISTENT is added to vb2_queue::dma_attrs.


Please can you point to where DMA_ATTR_NON_CONSISTENT does anything at 
all on arm64?


Also, I posit that videobuf2 is not actually relying on 
DMA_ATTR_NON_CONSISTENT anyway, since it's clearly not using it properly:


"By using this API, you are guaranteeing to the platform
that you have all the correct and necessary sync points for this memory
in the driver should it choose to return non-consistent memory."

$ git grep dma_cache_sync drivers/media
$

Robin.


[1] https://elixir.bootlin.com/linux/v5.9-rc1/source/kernel/dma/mapping.c#L341

When removing features from generic kernel code, I'd suggest first
providing viable alternatives for its users, rather than killing the
users altogether.

Given the above, I'm afraid I have to NAK this.

Best regards,
Tomasz



Signed-off-by: Christoph Hellwig 
---
  .../userspace-api/media/v4l/buffer.rst| 17 -
  .../media/v4l/vidioc-reqbufs.rst  |  1 -
  .../media/common/videobuf2/videobuf2-core.c   | 36 +--
  .../common/videobuf2/videobuf2-dma-contig.c   | 19 --
  .../media/common/videobuf2/videobuf2-dma-sg.c |  3 +-
  .../media/common/videobuf2/videobuf2-v4l2.c   | 12 ---
  include/media/videobuf2-core.h|  3 +-
  include/uapi/linux/videodev2.h|  2 --
  8 files changed, 3 insertions(+), 90 deletions(-)

diff --git a/Documentation/userspace-api/media/v4l/buffer.rst 
b/Documentation/userspace-api/media/v4l/buffer.rst
index 57e752aaf414a7..2044ed13cd9d7d 100644
--- a/Documentation/userspace-api/media/v4l/buffer.rst
+++ b/Documentation/userspace-api/media/v4l/buffer.rst
@@ -701,23 +701,6 @@ Memory Consistency Flags
  :stub-columns: 0
  :widths:   3 1 4

-* .. _`V4L2-FLAG-MEMORY-NON-CONSISTENT`:
-
-  - ``V4L2_FLAG_MEMORY_NON_CONSISTENT``
-  - 0x0001
-  - A buffer is allocated either in consistent (it will be automatically
-   coherent between the CPU and the bus) or non-consistent memory. The
-   latter can provide performance gains, for instance the CPU cache
-   sync/flush operations can be avoided if the buffer is accessed by the
-   corresponding device only and the CPU does not read/write to/from that
-   buffer. However, this requires extra care from the driver -- it must
-   guarantee memory consistency by issuing a cache flush/sync when
-   consistency is needed. If this flag is set V4L2 will attempt to
-   allocate the buffer in non-consistent memory. The flag takes effect
-   only if the buffer is used for :ref:`memory mapping ` I/O and the
-   queue reports the :ref:`V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS
-   ` capability.
-
  .. c:type:: v4l2_memory

  enum v4l2_memory
diff --git a/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst 
b/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
index 75d894d9c36c42..3180c111d368ee 100644
--- a/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
@@ -169,7 +169,6 @@ aborting or finishing any DMA in progress, an implicit
- This capability is set by the driver to indicate that the queue 
supports
  cache and memory management hints. However, it's only valid when the
  queue is used for :ref:`memory mapping ` streaming I/O. See
-:ref:`V4L2_FLAG_MEMORY_NON_CONSISTENT 
`,
  :ref:`V4L2_BUF_FLAG_NO_CACHE_INVALIDATE 
` and
  :ref:`V4L2_BUF_FLAG_NO_CACHE_CLEAN `.

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c 
b/drivers/media/common/videobuf2/videobuf2-core.c
index f544d3393e9d6b..66a41cef33c1b1 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -721,39 +721,14 @@ int vb2_verify_memory_type(struct vb2_queue *q,
  }
  EXPORT_SYMBOL(vb2_verify_memory_type);

-static void set_queue_consistency(struct vb2_queue *q, bool consistent_mem)
-{
-   q->dma_attrs &= ~DMA_ATTR_NON_CONSISTENT;
-
-   if (!vb2_queue_allows_cache_hints(q))
-   return;
-   if (!

Re: [Nouveau] [PATCH 08/11] drm/nouveau: tegra: Skip IOMMU initialization if already attached

2019-09-16 Thread Robin Murphy


On 16/09/2019 16:57, Thierry Reding wrote:

On Mon, Sep 16, 2019 at 04:29:18PM +0100, Robin Murphy wrote:

Hi Thierry,

On 16/09/2019 16:04, Thierry Reding wrote:

From: Thierry Reding 

If the GPU is already attached to an IOMMU, don't detach it and setup an
explicit IOMMU domain. Since Nouveau can now properly handle the case of
the DMA API being backed by an IOMMU, just continue using the DMA API.

Signed-off-by: Thierry Reding 
---
   .../drm/nouveau/nvkm/engine/device/tegra.c| 19 +++
   1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c 
b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
index d0d52c1d4aee..fc652aaa41c7 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
@@ -23,10 +23,6 @@
   #ifdef CONFIG_NOUVEAU_PLATFORM_DRIVER
   #include "priv.h"
-#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
-#include 
-#endif
-
   static int
   nvkm_device_tegra_power_up(struct nvkm_device_tegra *tdev)
   {
@@ -109,14 +105,13 @@ nvkm_device_tegra_probe_iommu(struct nvkm_device_tegra 
*tdev)
unsigned long pgsize_bitmap;
int ret;
-#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
-   if (dev->archdata.mapping) {
-   struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
-
-   arm_iommu_detach_device(dev);
-   arm_iommu_release_mapping(mapping);
-   }
-#endif
+   /*
+* Skip explicit IOMMU initialization if the GPU is already attached
+* to an IOMMU domain. This can happen if the DMA API is backed by an
+* IOMMU.
+*/
+   if (iommu_get_domain_for_dev(dev))
+   return;


Beware of "iommu.passthrough=1" - you could get a valid default domain here
yet still have direct/SWIOTLB DMA ops. I guess you probably want to
double-check the domain type as well.


Good point. An earlier version of this patch had an additional check for
IOMMU_DOMAIN_DMA, but then that failed on 32-bit ARM because there the
DMA API can also use IOMMU_DOMAIN_UNMANAGED type domains. Checking for
IOMMU_DOMAIN_IDENTIFY should be safe, though. That doesn't seem to
appear in arch/arm, arch/arm64 or drivers/iommu/dma-iommu.c.


Right, "domain && domain->type != IOMMU_DOMAIN_IDENTITY" should be 
sufficient to answer "is the DMA layer managing my address space for 
me?" unless and until some massive API change happens (which I certainly 
don't foresee).


Robin.
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH 08/11] drm/nouveau: tegra: Skip IOMMU initialization if already attached

2019-09-16 Thread Robin Murphy


Hi Thierry,

On 16/09/2019 16:04, Thierry Reding wrote:

From: Thierry Reding 

If the GPU is already attached to an IOMMU, don't detach it and setup an
explicit IOMMU domain. Since Nouveau can now properly handle the case of
the DMA API being backed by an IOMMU, just continue using the DMA API.

Signed-off-by: Thierry Reding 
---
  .../drm/nouveau/nvkm/engine/device/tegra.c| 19 +++
  1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c 
b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
index d0d52c1d4aee..fc652aaa41c7 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
@@ -23,10 +23,6 @@
  #ifdef CONFIG_NOUVEAU_PLATFORM_DRIVER
  #include "priv.h"
  
-#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)

-#include 
-#endif
-
  static int
  nvkm_device_tegra_power_up(struct nvkm_device_tegra *tdev)
  {
@@ -109,14 +105,13 @@ nvkm_device_tegra_probe_iommu(struct nvkm_device_tegra 
*tdev)
unsigned long pgsize_bitmap;
int ret;
  
-#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)

-   if (dev->archdata.mapping) {
-   struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
-
-   arm_iommu_detach_device(dev);
-   arm_iommu_release_mapping(mapping);
-   }
-#endif
+   /*
+* Skip explicit IOMMU initialization if the GPU is already attached
+* to an IOMMU domain. This can happen if the DMA API is backed by an
+* IOMMU.
+*/
+   if (iommu_get_domain_for_dev(dev))
+   return;


Beware of "iommu.passthrough=1" - you could get a valid default domain 
here yet still have direct/SWIOTLB DMA ops. I guess you probably want to 
double-check the domain type as well.


Robin.

  
  	if (!tdev->func->iommu_bit)

return;


___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: DMA-API: cacheline tracking ENOMEM, dma-debug disabled due to nouveau ?

2019-08-15 Thread Robin Murphy


On 15/08/2019 14:35, Christoph Hellwig wrote:

On Wed, Aug 14, 2019 at 07:49:27PM +0200, Daniel Vetter wrote:

On Wed, Aug 14, 2019 at 04:50:33PM +0200, Corentin Labbe wrote:

Hello

Since lot of release (at least since 4.19), I hit the following error message:
DMA-API: cacheline tracking ENOMEM, dma-debug disabled

After hitting that, I try to check who is creating so many DMA mapping and see:
cat /sys/kernel/debug/dma-api/dump | cut -d' ' -f2 | sort | uniq -c
   6 ahci
 257 e1000e
   6 ehci-pci
5891 nouveau
  24 uhci_hcd

Does nouveau having this high number of DMA mapping is normal ?


Yeah seems perfectly fine for a gpu.


That is a lot and apparently overwhelm the dma-debug tracking.  Robin
rewrote this code in Linux 4.21 to work a little better, so I'm curious
why this might have changes in 4.19, as dma-debug did not change at
all there.


FWIW, the cacheline tracking entries are a separate thing from the 
dma-debug entries that I rejigged - judging by those numbers there 
should still be plenty of free dma-debug entries, but for some reason it 
has failed to extend the radix tree :/


Robin.

Re: [Nouveau] [RFC PATCH v1 0/6] Resolve unwanted DMA backing with IOMMU

2018-08-23 Thread Robin Murphy


On 15/08/18 20:56, Dmitry Osipenko wrote:

On Friday, 3 August 2018 18:43:41 MSK Robin Murphy wrote:

On 02/08/18 19:24, Dmitry Osipenko wrote:

On Friday, 27 July 2018 20:16:53 MSK Dmitry Osipenko wrote:

On Friday, 27 July 2018 20:03:26 MSK Jordan Crouse wrote:

On Fri, Jul 27, 2018 at 05:02:37PM +0100, Robin Murphy wrote:

On 27/07/18 15:10, Dmitry Osipenko wrote:

On Friday, 27 July 2018 12:03:28 MSK Will Deacon wrote:

On Fri, Jul 27, 2018 at 10:25:13AM +0200, Joerg Roedel wrote:

On Fri, Jul 27, 2018 at 02:16:18AM +0300, Dmitry Osipenko wrote:

The proposed solution adds a new option to the base device driver
structure that allows device drivers to explicitly convey to the
drivers
core that the implicit IOMMU backing for devices must not happen.


Why is IOMMU mapping a problem for the Tegra GPU driver?

If we add something like this then it should not be the choice of
the
device driver, but of the user and/or the firmware.


Agreed, and it would still need somebody to configure an identity
domain
so
that transactions aren't aborted immediately. We currently allow the
identity domain to be used by default via a command-line option, so I
guess
we'd need a way for firmware to request that on a per-device basis.


The IOMMU mapping itself is not a problem, the problem is the
management
of
the IOMMU. For Tegra we don't want anything to intrude into the IOMMU
activities because:

1) GPU HW require additional configuration for the IOMMU usage and
dumb
mapping of the allocations simply doesn't work.


Generally, that's already handled by the DRM drivers allocating
their own unmanaged domains. The only problem we really need to
solve in that regard is that currently the device DMA ops don't get
updated when moving away from the managed domain. That's been OK for
the VFIO case where the device is bound to a different driver which
we know won't make any explicit DMA API calls, but for the more
general case of IOMMU-aware drivers we could certainly do with a bit
of cooperation between the IOMMU API, DMA API, and arch code to
update the DMA ops dynamically to cope with intermediate subsystems
making DMA API calls on behalf of devices they don't know the
intimate details of.


2) Older Tegra generations have a limited resource and capabilities in
regards to IOMMU usage, allocating IOMMU domain per-device is just
impossible for example.

3) HW performs context switches and so particular allocations have to
be
assigned to a particular contexts IOMMU domain.


I understand Qualcomm SoCs have a similar thing too, and AFAICS that
case just doesn't fit into the current API model at all. We need the
IOMMU driver to somehow know about the specific details of which
devices have magic associations with specific contexts, and we
almost certainly need a more expressive interface than
iommu_domain_alloc() to have any hope of reliable results.


This is correct for Qualcomm GPUs - The GPU hardware context switching
requires a specific context and there are some restrictions around
secure contexts as well.

We don't really care if the DMA attaches to a context just as long as it
doesn't attach to the one(s) we care about. Perhaps a "valid context"
mask
would work in from the DT or the device struct to give the subsystems a
clue as to which domains they were allowed to use. I recognize that
there
isn't a one-size-fits-all solution to this problem so I'm open to
different
ideas.


Designating whether implicit IOMMU backing is appropriate for a device
via
device-tree property sounds a bit awkward because that will be a kinda
software description (of a custom Linux driver model), while device-tree
is
supposed to describe HW.

What about to grant IOMMU drivers with ability to decide whether the
implicit backing for a device is appropriate? Like this:

bool implicit_iommu_for_dma_is_allowed(struct device *dev)
{

const struct iommu_ops *ops = dev->bus->iommu_ops;
struct iommu_group *group;

group = iommu_group_get(dev);
if (!group)

return NULL;

iommu_group_put(group);

if (!ops->implicit_iommu_for_dma_is_allowed)

return true;

return ops->implicit_iommu_for_dma_is_allowed(dev);

}

Then arch_setup_dma_ops() could have a clue whether implicit IOMMU
backing
for a device is appropriate.


Guys, does it sound good to you or maybe you have something else on your
mind? Even if it's not an ideal solution, it fixes the immediate problem
and should be good enough for the starter.


To me that looks like a step ion the wrong direction that won't help at
all in actually addressing the underlying issues.

If the GPU driver wants to explicitly control IOMMU mappings instead of
relying on the IOMMU_DOMAIN_DMA abstraction, then it should use its own
unmanaged domain. At that point it shouldn't matter if

Re: [Nouveau] [RFC PATCH v1 0/6] Resolve unwanted DMA backing with IOMMU

2018-08-23 Thread Robin Murphy


On 02/08/18 19:24, Dmitry Osipenko wrote:

On Friday, 27 July 2018 20:16:53 MSK Dmitry Osipenko wrote:

On Friday, 27 July 2018 20:03:26 MSK Jordan Crouse wrote:

On Fri, Jul 27, 2018 at 05:02:37PM +0100, Robin Murphy wrote:

On 27/07/18 15:10, Dmitry Osipenko wrote:

On Friday, 27 July 2018 12:03:28 MSK Will Deacon wrote:

On Fri, Jul 27, 2018 at 10:25:13AM +0200, Joerg Roedel wrote:

On Fri, Jul 27, 2018 at 02:16:18AM +0300, Dmitry Osipenko wrote:

The proposed solution adds a new option to the base device driver
structure that allows device drivers to explicitly convey to the
drivers
core that the implicit IOMMU backing for devices must not happen.


Why is IOMMU mapping a problem for the Tegra GPU driver?

If we add something like this then it should not be the choice of the
device driver, but of the user and/or the firmware.


Agreed, and it would still need somebody to configure an identity
domain
so
that transactions aren't aborted immediately. We currently allow the
identity domain to be used by default via a command-line option, so I
guess
we'd need a way for firmware to request that on a per-device basis.


The IOMMU mapping itself is not a problem, the problem is the
management
of
the IOMMU. For Tegra we don't want anything to intrude into the IOMMU
activities because:

1) GPU HW require additional configuration for the IOMMU usage and dumb
mapping of the allocations simply doesn't work.


Generally, that's already handled by the DRM drivers allocating
their own unmanaged domains. The only problem we really need to
solve in that regard is that currently the device DMA ops don't get
updated when moving away from the managed domain. That's been OK for
the VFIO case where the device is bound to a different driver which
we know won't make any explicit DMA API calls, but for the more
general case of IOMMU-aware drivers we could certainly do with a bit
of cooperation between the IOMMU API, DMA API, and arch code to
update the DMA ops dynamically to cope with intermediate subsystems
making DMA API calls on behalf of devices they don't know the
intimate details of.


2) Older Tegra generations have a limited resource and capabilities in
regards to IOMMU usage, allocating IOMMU domain per-device is just
impossible for example.

3) HW performs context switches and so particular allocations have to
be
assigned to a particular contexts IOMMU domain.


I understand Qualcomm SoCs have a similar thing too, and AFAICS that
case just doesn't fit into the current API model at all. We need the
IOMMU driver to somehow know about the specific details of which
devices have magic associations with specific contexts, and we
almost certainly need a more expressive interface than
iommu_domain_alloc() to have any hope of reliable results.


This is correct for Qualcomm GPUs - The GPU hardware context switching
requires a specific context and there are some restrictions around
secure contexts as well.

We don't really care if the DMA attaches to a context just as long as it
doesn't attach to the one(s) we care about. Perhaps a "valid context" mask
would work in from the DT or the device struct to give the subsystems a
clue as to which domains they were allowed to use. I recognize that there
isn't a one-size-fits-all solution to this problem so I'm open to
different
ideas.


Designating whether implicit IOMMU backing is appropriate for a device via
device-tree property sounds a bit awkward because that will be a kinda
software description (of a custom Linux driver model), while device-tree is
supposed to describe HW.

What about to grant IOMMU drivers with ability to decide whether the
implicit backing for a device is appropriate? Like this:

bool implicit_iommu_for_dma_is_allowed(struct device *dev)
{
const struct iommu_ops *ops = dev->bus->iommu_ops;
struct iommu_group *group;

group = iommu_group_get(dev);
if (!group)
return NULL;

iommu_group_put(group);

if (!ops->implicit_iommu_for_dma_is_allowed)
return true;

return ops->implicit_iommu_for_dma_is_allowed(dev);
}

Then arch_setup_dma_ops() could have a clue whether implicit IOMMU backing
for a device is appropriate.


Guys, does it sound good to you or maybe you have something else on your mind?
Even if it's not an ideal solution, it fixes the immediate problem and should
be good enough for the starter.


To me that looks like a step ion the wrong direction that won't help at 
all in actually addressing the underlying issues.


If the GPU driver wants to explicitly control IOMMU mappings instead of 
relying on the IOMMU_DOMAIN_DMA abstraction, then it should use its own 
unmanaged domain. At that point it shouldn't matter if a DMA ops domain 
was allocated, since the GPU device will no longer be attached to it. 
Yes, there may be some improvements to make like having unus

Re: [Nouveau] [RFC PATCH v1 0/6] Resolve unwanted DMA backing with IOMMU

2018-07-27 Thread Robin Murphy


On 27/07/18 15:10, Dmitry Osipenko wrote:

On Friday, 27 July 2018 12:03:28 MSK Will Deacon wrote:

On Fri, Jul 27, 2018 at 10:25:13AM +0200, Joerg Roedel wrote:

On Fri, Jul 27, 2018 at 02:16:18AM +0300, Dmitry Osipenko wrote:

The proposed solution adds a new option to the base device driver
structure that allows device drivers to explicitly convey to the drivers
core that the implicit IOMMU backing for devices must not happen.


Why is IOMMU mapping a problem for the Tegra GPU driver?

If we add something like this then it should not be the choice of the
device driver, but of the user and/or the firmware.


Agreed, and it would still need somebody to configure an identity domain so
that transactions aren't aborted immediately. We currently allow the
identity domain to be used by default via a command-line option, so I guess
we'd need a way for firmware to request that on a per-device basis.


The IOMMU mapping itself is not a problem, the problem is the management of
the IOMMU. For Tegra we don't want anything to intrude into the IOMMU
activities because:

1) GPU HW require additional configuration for the IOMMU usage and dumb
mapping of the allocations simply doesn't work.


Generally, that's already handled by the DRM drivers allocating their 
own unmanaged domains. The only problem we really need to solve in that 
regard is that currently the device DMA ops don't get updated when 
moving away from the managed domain. That's been OK for the VFIO case 
where the device is bound to a different driver which we know won't make 
any explicit DMA API calls, but for the more general case of IOMMU-aware 
drivers we could certainly do with a bit of cooperation between the 
IOMMU API, DMA API, and arch code to update the DMA ops dynamically to 
cope with intermediate subsystems making DMA API calls on behalf of 
devices they don't know the intimate details of.



2) Older Tegra generations have a limited resource and capabilities in regards
to IOMMU usage, allocating IOMMU domain per-device is just impossible for
example.

3) HW performs context switches and so particular allocations have to be
assigned to a particular contexts IOMMU domain.


I understand Qualcomm SoCs have a similar thing too, and AFAICS that 
case just doesn't fit into the current API model at all. We need the 
IOMMU driver to somehow know about the specific details of which devices 
have magic associations with specific contexts, and we almost certainly 
need a more expressive interface than iommu_domain_alloc() to have any 
hope of reliable results.


Robin.


Some of the above is due to a SW driver model (and its work-in-progress
status), other is due to a HW model. So essentially we need a way for a driver
to tell the core not to mess with IOMMU stuff of drivers device behind the
drivers back.

I'm not sure what you guys are meaning by the "firmware", could you elaborate
please? Do you mean the Open Firmware and hence the devicetree or what?



___
iommu mailing list
io...@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v4 2/2] drm/nouveau: tegra: Detach from ARM DMA/IOMMU mapping

2018-05-31 Thread Robin Murphy


On 30/05/18 15:06, Thierry Reding wrote:

From: Thierry Reding 

Depending on the kernel configuration, early ARM architecture setup code
may have attached the GPU to a DMA/IOMMU mapping that transparently uses
the IOMMU to back the DMA API. Tegra requires special handling for IOMMU
backed buffers (a special bit in the GPU's MMU page tables indicates the
memory path to take: via the SMMU or directly to the memory controller).
Transparently backing DMA memory with an IOMMU prevents Nouveau from
properly handling such memory accesses and causes memory access faults.

As a side-note: buffers other than those allocated in instance memory
don't need to be physically contiguous from the GPU's perspective since
the GPU can map them into contiguous buffers using its own MMU. Mapping
these buffers through the IOMMU is unnecessary and will even lead to
performance degradation because of the additional translation. One
exception to this are compressible buffers which need large pages. In
order to enable these large pages, multiple small pages will have to be
combined into one large (I/O virtually contiguous) mapping via the
IOMMU. However, that is a topic outside the scope of this fix and isn't
currently supported. An implementation will want to explicitly create
these large pages in the Nouveau driver, so detaching from a DMA/IOMMU
mapping would still be required.

Signed-off-by: Thierry Reding 
---
Changes in v4:
- use existing APIs to detach from a DMA/IOMMU mapping

Changes in v3:
- clarify the use of IOMMU mapping for compressible buffers
- squash multiple patches into this

  drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c | 13 +
  1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c 
b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
index 78597da6313a..0e372a190d3f 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
@@ -23,6 +23,10 @@
  #ifdef CONFIG_NOUVEAU_PLATFORM_DRIVER
  #include "priv.h"
  
+#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)

+#include 
+#endif
+
  static int
  nvkm_device_tegra_power_up(struct nvkm_device_tegra *tdev)
  {
@@ -105,6 +109,15 @@ nvkm_device_tegra_probe_iommu(struct nvkm_device_tegra 
*tdev)
unsigned long pgsize_bitmap;
int ret;
  
+#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)

+   if (dev->archdata.mapping) {
+   struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);


Nit: there's arguably little point using the helper here after you've 
already shattered the illusion by poking dev->archdata.mapping directly, 
but I guess this disappears again anyway once the refcounting gets 
sorted out and the mapping releases itself properly, so:


Reviewed-by: Robin Murphy 


+
+   arm_iommu_detach_device(dev);
+   arm_iommu_release_mapping(mapping);
+   }
+#endif
+
if (!tdev->func->iommu_bit)
return;
  


___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v4 1/2] ARM: dma-mapping: Set proper DMA ops in arm_iommu_detach_device()

2018-05-31 Thread Robin Murphy


On 30/05/18 15:06, Thierry Reding wrote:

From: Thierry Reding 

Instead of setting the DMA ops pointer to NULL, set the correct,
non-IOMMU ops depending on the device's coherency setting.


It looks like it's probably been 4 or 5 years since that became subtly 
wrong by virtue of the landscape changing around it, but it's clearly 
not enough of a problem to consider stable backports :)


Reviewed-by: Robin Murphy 


Signed-off-by: Thierry Reding 
---
Changes in v4:
- new patch to fix existing arm_iommu_detach_device() to do what we need

  arch/arm/mm/dma-mapping.c | 12 ++--
  1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index af27f1c22d93..87a0037574e4 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1151,6 +1151,11 @@ int arm_dma_supported(struct device *dev, u64 mask)
return __dma_supported(dev, mask, false);
  }
  
+static const struct dma_map_ops *arm_get_dma_map_ops(bool coherent)

+{
+   return coherent ? &arm_coherent_dma_ops : &arm_dma_ops;
+}
+
  #ifdef CONFIG_ARM_DMA_USE_IOMMU
  
  static int __dma_info_to_prot(enum dma_data_direction dir, unsigned long attrs)

@@ -2296,7 +2301,7 @@ void arm_iommu_detach_device(struct device *dev)
iommu_detach_device(mapping->domain, dev);
kref_put(&mapping->kref, release_iommu_mapping);
to_dma_iommu_mapping(dev) = NULL;
-   set_dma_ops(dev, NULL);
+   set_dma_ops(dev, arm_get_dma_map_ops(dev->archdata.dma_coherent));
  
  	pr_debug("Detached IOMMU controller from %s device.\n", dev_name(dev));

  }
@@ -2357,11 +2362,6 @@ static void arm_teardown_iommu_dma_ops(struct device 
*dev) { }
  
  #endif	/* CONFIG_ARM_DMA_USE_IOMMU */
  
-static const struct dma_map_ops *arm_get_dma_map_ops(bool coherent)

-{
-   return coherent ? &arm_coherent_dma_ops : &arm_dma_ops;
-}
-
  void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
const struct iommu_ops *iommu, bool coherent)
  {


___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v3 2/2] drm/nouveau: tegra: Detach from ARM DMA/IOMMU mapping

2018-05-30 Thread Robin Murphy


On 30/05/18 14:41, Thierry Reding wrote:

On Wed, May 30, 2018 at 02:30:51PM +0100, Robin Murphy wrote:

On 30/05/18 14:00, Thierry Reding wrote:

On Wed, May 30, 2018 at 11:30:25AM +0100, Robin Murphy wrote:

On 30/05/18 09:03, Thierry Reding wrote:

From: Thierry Reding 

Depending on the kernel configuration, early ARM architecture setup code
may have attached the GPU to a DMA/IOMMU mapping that transparently uses
the IOMMU to back the DMA API. Tegra requires special handling for IOMMU
backed buffers (a special bit in the GPU's MMU page tables indicates the
memory path to take: via the SMMU or directly to the memory controller).
Transparently backing DMA memory with an IOMMU prevents Nouveau from
properly handling such memory accesses and causes memory access faults.

As a side-note: buffers other than those allocated in instance memory
don't need to be physically contiguous from the GPU's perspective since
the GPU can map them into contiguous buffers using its own MMU. Mapping
these buffers through the IOMMU is unnecessary and will even lead to
performance degradation because of the additional translation. One
exception to this are compressible buffers which need large pages. In
order to enable these large pages, multiple small pages will have to be
combined into one large (I/O virtually contiguous) mapping via the
IOMMU. However, that is a topic outside the scope of this fix and isn't
currently supported. An implementation will want to explicitly create
these large pages in the Nouveau driver, so detaching from a DMA/IOMMU
mapping would still be required.


I wonder if it might make sense to have a hook in iommu_attach_device() to
notify the arch DMA API code when moving devices between unmanaged and DMA
ops domains? That seems like it might be the most low-impact way to address
the overall problem long-term.


Signed-off-by: Thierry Reding 
---
Changes in v3:
- clarify the use of IOMMU mapping for compressible buffers
- squash multiple patches into this

drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c | 5 +
1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c 
b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
index 78597da6313a..d0538af1b967 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
@@ -105,6 +105,11 @@ nvkm_device_tegra_probe_iommu(struct nvkm_device_tegra 
*tdev)
unsigned long pgsize_bitmap;
int ret;
+#if IS_ENABLED(CONFIG_ARM)


Wouldn't CONFIG_ARM_DMA_USE_IOMMU be even more appropriate?


Not necessarily. arm_dma_iommu_detach_device() is always defined on ARM,
only with CONFIG_ARM_DMA_USE_IOMMU=n it will be empty. So this check is
a guard to make sure we don't call the function when it isn't available,
but it may still not do anything.


Calling a function under condition A, which only does anything under
condition B, when B depends on A, is identical in behaviour to only calling
the function under condition B, except needlessly harder to follow.


+   /* make sure we can use the IOMMU exclusively */
+   arm_dma_iommu_detach_device(dev);


As before, I would just use the existing infrastructure the same way the
Exynos DRM driver currently does in __exynos_iommu_attach() (albeit without
then reattaching to another DMA ops mapping).


That's pretty much what I initially did and which was shot down by
Christoph. As I said earlier, at this point I don't really care what
color the shed will be. Can you and Christoph come to an agreement
on what it should be?


What I was getting at is that arm_iommu_detach_device() already *is* the
exact function Christoph was asking for, it just needs a minor fix instead
of adding explicit set_dma_ops() fiddling at its callsites which only
obfuscates the fact that it's supposed to be responsible for resetting the
device's DMA ops already.


It still has the downside of callers having to explicitly check for the
existence of a mapping, otherwise they'll cause a warning to be printed
to the kernel log.


Or we could look at the way it's actually used, and reconsider whether 
the warning is really appropriate. That's always an option ;)


Robin.


That's not all that bad, though. I'll prepare version 4 with those
changes.

Thierry


___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v3 1/2] ARM: dma-mapping: Implement arm_dma_iommu_detach_device()

2018-05-30 Thread Robin Murphy


On 30/05/18 14:12, Thierry Reding wrote:

On Wed, May 30, 2018 at 02:54:46PM +0200, Thierry Reding wrote:

On Wed, May 30, 2018 at 10:59:30AM +0100, Robin Murphy wrote:

On 30/05/18 09:03, Thierry Reding wrote:

From: Thierry Reding 

Implement this function to enable drivers from detaching from any IOMMU
domains that architecture code might have attached them to so that they
can take exclusive control of the IOMMU via the IOMMU API.

Signed-off-by: Thierry Reding 
---
Changes in v3:
- make API 32-bit ARM specific
- avoid extra local variable

Changes in v2:
- fix compilation

   arch/arm/include/asm/dma-mapping.h |  3 +++
   arch/arm/mm/dma-mapping-nommu.c|  4 
   arch/arm/mm/dma-mapping.c  | 16 
   3 files changed, 23 insertions(+)

diff --git a/arch/arm/include/asm/dma-mapping.h 
b/arch/arm/include/asm/dma-mapping.h
index 8436f6ade57d..5960e9f3a9d0 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -103,6 +103,9 @@ extern void arch_setup_dma_ops(struct device *dev, u64 
dma_base, u64 size,
   #define arch_teardown_dma_ops arch_teardown_dma_ops
   extern void arch_teardown_dma_ops(struct device *dev);
+#define arm_dma_iommu_detach_device arm_dma_iommu_detach_device
+extern void arm_dma_iommu_detach_device(struct device *dev);
+
   /* do not use this function in a driver */
   static inline bool is_device_dma_coherent(struct device *dev)
   {
diff --git a/arch/arm/mm/dma-mapping-nommu.c b/arch/arm/mm/dma-mapping-nommu.c
index f448a0663b10..eb781369377b 100644
--- a/arch/arm/mm/dma-mapping-nommu.c
+++ b/arch/arm/mm/dma-mapping-nommu.c
@@ -241,3 +241,7 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, 
u64 size,
   void arch_teardown_dma_ops(struct device *dev)
   {
   }
+
+void arm_dma_iommu_detach_device(struct device *dev)
+{
+}
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index af27f1c22d93..6d8af08b3e7d 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -2400,3 +2400,19 @@ void arch_teardown_dma_ops(struct device *dev)
arm_teardown_iommu_dma_ops(dev);
   }
+
+void arm_dma_iommu_detach_device(struct device *dev)
+{
+#ifdef CONFIG_ARM_DMA_USE_IOMMU
+   struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
+
+   if (!mapping)
+   return;
+
+   arm_iommu_release_mapping(mapping);


Potentially freeing the mapping before you try to operate on it is never the
best idea. Plus arm_iommu_detach_device() already releases a reference
appropriately anyway, so it's a double-free.


But the reference released by arm_iommu_detach_device() is to balance
out the reference acquired by arm_iommu_attach_device(), isn't it? In
the above, the arm_iommu_release_mapping() is supposed to drop the
final reference which was obtained by arm_iommu_create_mapping(). The
mapping shouldn't go away irrespective of the order in which these
will be called.


Going over the DMA/IOMMU code I just remembered that I drew inspiration
from arm_teardown_iommu_dma_ops() for the initial proposal which also
calls both arm_iommu_detach_device() and arm_iommu_release_mapping().
That said, one other possibility to implement this would be to export
the 32-bit and 64-bit ARM implementations of arch_teardown_dma_ops()
and use that instead. linux/dma-mapping.h implements a stub for
architectures that don't provide one, so it should work without any
#ifdef guards.

That combined with the set_dma_ops() fix in arm_iommu_detach_device()
should fix this pretty nicely.


OK, having a second look at the ARM code I see I had indeed overlooked 
that extra reference held until arm_teardown_iommu_dma_ops() - mea culpa 
- but frankly that looks wrong anyway, as it basically defeats the point 
of refcounting the mapping at all. AFAICS arm_setup_iommu_dma_ops() 
should just be made to behave 'normally' by unconditionally dropping the 
initial reference after calling __arm_iommu_attach_device(), then we 
don't need all these odd and confusing release calls dotted around at all.


Robin.
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v3 2/2] drm/nouveau: tegra: Detach from ARM DMA/IOMMU mapping

2018-05-30 Thread Robin Murphy


On 30/05/18 14:00, Thierry Reding wrote:

On Wed, May 30, 2018 at 11:30:25AM +0100, Robin Murphy wrote:

On 30/05/18 09:03, Thierry Reding wrote:

From: Thierry Reding 

Depending on the kernel configuration, early ARM architecture setup code
may have attached the GPU to a DMA/IOMMU mapping that transparently uses
the IOMMU to back the DMA API. Tegra requires special handling for IOMMU
backed buffers (a special bit in the GPU's MMU page tables indicates the
memory path to take: via the SMMU or directly to the memory controller).
Transparently backing DMA memory with an IOMMU prevents Nouveau from
properly handling such memory accesses and causes memory access faults.

As a side-note: buffers other than those allocated in instance memory
don't need to be physically contiguous from the GPU's perspective since
the GPU can map them into contiguous buffers using its own MMU. Mapping
these buffers through the IOMMU is unnecessary and will even lead to
performance degradation because of the additional translation. One
exception to this are compressible buffers which need large pages. In
order to enable these large pages, multiple small pages will have to be
combined into one large (I/O virtually contiguous) mapping via the
IOMMU. However, that is a topic outside the scope of this fix and isn't
currently supported. An implementation will want to explicitly create
these large pages in the Nouveau driver, so detaching from a DMA/IOMMU
mapping would still be required.


I wonder if it might make sense to have a hook in iommu_attach_device() to
notify the arch DMA API code when moving devices between unmanaged and DMA
ops domains? That seems like it might be the most low-impact way to address
the overall problem long-term.


Signed-off-by: Thierry Reding 
---
Changes in v3:
- clarify the use of IOMMU mapping for compressible buffers
- squash multiple patches into this

   drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c | 5 +
   1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c 
b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
index 78597da6313a..d0538af1b967 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
@@ -105,6 +105,11 @@ nvkm_device_tegra_probe_iommu(struct nvkm_device_tegra 
*tdev)
unsigned long pgsize_bitmap;
int ret;
+#if IS_ENABLED(CONFIG_ARM)


Wouldn't CONFIG_ARM_DMA_USE_IOMMU be even more appropriate?


Not necessarily. arm_dma_iommu_detach_device() is always defined on ARM,
only with CONFIG_ARM_DMA_USE_IOMMU=n it will be empty. So this check is
a guard to make sure we don't call the function when it isn't available,
but it may still not do anything.


Calling a function under condition A, which only does anything under 
condition B, when B depends on A, is identical in behaviour to only 
calling the function under condition B, except needlessly harder to follow.



+   /* make sure we can use the IOMMU exclusively */
+   arm_dma_iommu_detach_device(dev);


As before, I would just use the existing infrastructure the same way the
Exynos DRM driver currently does in __exynos_iommu_attach() (albeit without
then reattaching to another DMA ops mapping).


That's pretty much what I initially did and which was shot down by
Christoph. As I said earlier, at this point I don't really care what
color the shed will be. Can you and Christoph come to an agreement
on what it should be?


What I was getting at is that arm_iommu_detach_device() already *is* the 
exact function Christoph was asking for, it just needs a minor fix 
instead of adding explicit set_dma_ops() fiddling at its callsites which 
only obfuscates the fact that it's supposed to be responsible for 
resetting the device's DMA ops already.


Robin.
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v3 2/2] drm/nouveau: tegra: Detach from ARM DMA/IOMMU mapping

2018-05-30 Thread Robin Murphy


On 30/05/18 09:03, Thierry Reding wrote:

From: Thierry Reding 

Depending on the kernel configuration, early ARM architecture setup code
may have attached the GPU to a DMA/IOMMU mapping that transparently uses
the IOMMU to back the DMA API. Tegra requires special handling for IOMMU
backed buffers (a special bit in the GPU's MMU page tables indicates the
memory path to take: via the SMMU or directly to the memory controller).
Transparently backing DMA memory with an IOMMU prevents Nouveau from
properly handling such memory accesses and causes memory access faults.

As a side-note: buffers other than those allocated in instance memory
don't need to be physically contiguous from the GPU's perspective since
the GPU can map them into contiguous buffers using its own MMU. Mapping
these buffers through the IOMMU is unnecessary and will even lead to
performance degradation because of the additional translation. One
exception to this are compressible buffers which need large pages. In
order to enable these large pages, multiple small pages will have to be
combined into one large (I/O virtually contiguous) mapping via the
IOMMU. However, that is a topic outside the scope of this fix and isn't
currently supported. An implementation will want to explicitly create
these large pages in the Nouveau driver, so detaching from a DMA/IOMMU
mapping would still be required.


I wonder if it might make sense to have a hook in iommu_attach_device() 
to notify the arch DMA API code when moving devices between unmanaged 
and DMA ops domains? That seems like it might be the most low-impact way 
to address the overall problem long-term.



Signed-off-by: Thierry Reding 
---
Changes in v3:
- clarify the use of IOMMU mapping for compressible buffers
- squash multiple patches into this

  drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c | 5 +
  1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c 
b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
index 78597da6313a..d0538af1b967 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
@@ -105,6 +105,11 @@ nvkm_device_tegra_probe_iommu(struct nvkm_device_tegra 
*tdev)
unsigned long pgsize_bitmap;
int ret;
  
+#if IS_ENABLED(CONFIG_ARM)


Wouldn't CONFIG_ARM_DMA_USE_IOMMU be even more appropriate?


+   /* make sure we can use the IOMMU exclusively */
+   arm_dma_iommu_detach_device(dev);


As before, I would just use the existing infrastructure the same way the 
Exynos DRM driver currently does in __exynos_iommu_attach() (albeit 
without then reattaching to another DMA ops mapping).


Robin.


+#endif
+
if (!tdev->func->iommu_bit)
return;
  


___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v3 1/2] ARM: dma-mapping: Implement arm_dma_iommu_detach_device()

2018-05-30 Thread Robin Murphy


On 30/05/18 09:03, Thierry Reding wrote:

From: Thierry Reding 

Implement this function to enable drivers from detaching from any IOMMU
domains that architecture code might have attached them to so that they
can take exclusive control of the IOMMU via the IOMMU API.

Signed-off-by: Thierry Reding 
---
Changes in v3:
- make API 32-bit ARM specific
- avoid extra local variable

Changes in v2:
- fix compilation

  arch/arm/include/asm/dma-mapping.h |  3 +++
  arch/arm/mm/dma-mapping-nommu.c|  4 
  arch/arm/mm/dma-mapping.c  | 16 
  3 files changed, 23 insertions(+)

diff --git a/arch/arm/include/asm/dma-mapping.h 
b/arch/arm/include/asm/dma-mapping.h
index 8436f6ade57d..5960e9f3a9d0 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -103,6 +103,9 @@ extern void arch_setup_dma_ops(struct device *dev, u64 
dma_base, u64 size,
  #define arch_teardown_dma_ops arch_teardown_dma_ops
  extern void arch_teardown_dma_ops(struct device *dev);
  
+#define arm_dma_iommu_detach_device arm_dma_iommu_detach_device

+extern void arm_dma_iommu_detach_device(struct device *dev);
+
  /* do not use this function in a driver */
  static inline bool is_device_dma_coherent(struct device *dev)
  {
diff --git a/arch/arm/mm/dma-mapping-nommu.c b/arch/arm/mm/dma-mapping-nommu.c
index f448a0663b10..eb781369377b 100644
--- a/arch/arm/mm/dma-mapping-nommu.c
+++ b/arch/arm/mm/dma-mapping-nommu.c
@@ -241,3 +241,7 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, 
u64 size,
  void arch_teardown_dma_ops(struct device *dev)
  {
  }
+
+void arm_dma_iommu_detach_device(struct device *dev)
+{
+}
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index af27f1c22d93..6d8af08b3e7d 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -2400,3 +2400,19 @@ void arch_teardown_dma_ops(struct device *dev)
  
  	arm_teardown_iommu_dma_ops(dev);

  }
+
+void arm_dma_iommu_detach_device(struct device *dev)
+{
+#ifdef CONFIG_ARM_DMA_USE_IOMMU
+   struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
+
+   if (!mapping)
+   return;
+
+   arm_iommu_release_mapping(mapping);


Potentially freeing the mapping before you try to operate on it is never 
the best idea. Plus arm_iommu_detach_device() already releases a 
reference appropriately anyway, so it's a double-free.



+   arm_iommu_detach_device(dev);
+
+   set_dma_ops(dev, arm_get_dma_map_ops(dev->archdata.dma_coherent));
+#endif
+}
+EXPORT_SYMBOL(arm_dma_iommu_detach_device);


I really don't see why we need an extra function that essentially just 
duplicates arm_iommu_detach_device(). The only real difference here is 
that here you reset the DMA ops more appropriately, but I think the 
existing function should be fixed to do that anyway, since 
set_dma_ops(dev, NULL) now just behaves as an unconditional fallback to 
the noncoherent arm_dma_ops, which clearly isn't always right.


Robin.
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v2 2/5] dma-mapping: Introduce dma_iommu_detach_device() API

2018-04-30 Thread Robin Murphy


On 30/04/18 13:12, Thierry Reding wrote:

On Mon, Apr 30, 2018 at 12:41:52PM +0100, Robin Murphy wrote:

On 30/04/18 12:02, Thierry Reding wrote:

On Thu, Apr 26, 2018 at 02:11:36PM +0200, Thierry Reding wrote:

On Wed, Apr 25, 2018 at 08:19:34AM -0700, Christoph Hellwig wrote:

On Wed, Apr 25, 2018 at 12:10:48PM +0200, Thierry Reding wrote:

From: Thierry Reding 

The dma_iommu_detach_device() API can be used by drivers to forcibly
detach a device from an IOMMU that architecture code might have attached
to. This is useful for drivers that need explicit control over the IOMMU
using the IOMMU API directly.


Given that no one else implements it making it a generic API seems
rather confusing.  For now I'd rename it to
arm_dma_iommu_detach_device() and only implement it in arm.


That'd be suboptimal because this code is used on both 32-bit and 64-bit
ARM. If we make the function 32-bit ARM specific then the driver code
would need to use an #ifdef to make sure compilation doesn't break on
64-bit ARM.


Do you still want me to make this ARM specific? While I haven't
encountered this issue on 64-bit ARM yet, I think it would happen there
as well, under the right circumstances. I could take a shot at
implementing the equivalent there (which means essentially implementing
it for drivers/iommu/dma-iommu.c and calling that from 64-bit ARM code).


It sounds like things are getting a bit backwards here: iommu-dma should
have nothing to do with this, since if you've explicitly attached the device
to your own IOMMU domain then you're already bypassing everything it knows
about and has control over. Arch code calling into iommu-dma to do something
which makes arch code not use iommu-dma makes very little sense.


My understanding is that iommu-dma will set up an IOMMU domain at device
probe time anyway. So even if attaching to an own IOMMU domain will end
up bypassing iommu-dma, we'd still want to clear up the IOMMU domain and
any associated resources, right?


The lifetime of a "proper" IOMMU API default domain is that of the 
iommu_group, so more or less between device_add() and device_del() from 
the perspective of a device in that group. IOW at least one level below 
what that device's driver should be messing with. The domain itself is 
just a little bit of memory and shouldn't have to occupy any hardware 
resources while it's not active (and yes, I know the ARM SMMU driver is 
currently a bit crap in that regard).


Robin.
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v2 2/5] dma-mapping: Introduce dma_iommu_detach_device() API

2018-04-30 Thread Robin Murphy


On 30/04/18 12:02, Thierry Reding wrote:

On Thu, Apr 26, 2018 at 02:11:36PM +0200, Thierry Reding wrote:

On Wed, Apr 25, 2018 at 08:19:34AM -0700, Christoph Hellwig wrote:

On Wed, Apr 25, 2018 at 12:10:48PM +0200, Thierry Reding wrote:

From: Thierry Reding 

The dma_iommu_detach_device() API can be used by drivers to forcibly
detach a device from an IOMMU that architecture code might have attached
to. This is useful for drivers that need explicit control over the IOMMU
using the IOMMU API directly.


Given that no one else implements it making it a generic API seems
rather confusing.  For now I'd rename it to
arm_dma_iommu_detach_device() and only implement it in arm.


That'd be suboptimal because this code is used on both 32-bit and 64-bit
ARM. If we make the function 32-bit ARM specific then the driver code
would need to use an #ifdef to make sure compilation doesn't break on
64-bit ARM.


Do you still want me to make this ARM specific? While I haven't
encountered this issue on 64-bit ARM yet, I think it would happen there
as well, under the right circumstances. I could take a shot at
implementing the equivalent there (which means essentially implementing
it for drivers/iommu/dma-iommu.c and calling that from 64-bit ARM code).


It sounds like things are getting a bit backwards here: iommu-dma should 
have nothing to do with this, since if you've explicitly attached the 
device to your own IOMMU domain then you're already bypassing everything 
it knows about and has control over. Arch code calling into iommu-dma to 
do something which makes arch code not use iommu-dma makes very little 
sense.


AFAICS the only aspect of arm_iommu_detach_device() which actually 
matters in this case is the set_dma_ops() bit, so what we're really 
after is a generic way for drivers to say "Hey, I actually have my own 
MMU (or want to control the one you already know about) so please give 
me direct DMA ops", which the arch code handles as appropriate (maybe 
it's also allowed to fail in cases like swiotlb=force or when there is 
RAM beyond the device's DMA mask).


Robin.
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v2] Revert "drm/nouveau/device/pci: set as non-CPU-coherent on ARM64"

2016-06-06 Thread Robin Murphy


On 06/06/16 08:11, Alexandre Courbot wrote:

From: Robin Murphy 

This reverts commit 1733a2ad36741b1812cf8b3f3037c28d0af53f50.

There is apparently something amiss with the way the TTM code handles
DMA buffers, which the above commit was attempting to work around for
arm64 systems with non-coherent PCI. Unfortunately, this completely
breaks systems *with* coherent PCI (which appear to be the majority).

Booting a plain arm64 defconfig + CONFIG_DRM + CONFIG_DRM_NOUVEAU on
a machine with a PCI GPU having coherent dma_map_ops (in this case a
7600GT card plugged into an ARM Juno board) results in a fatal crash:

[2.803438] nouveau :06:00.0: DRM: allocated 1024x768 fb: 0x9000, bo 
ffc976141c00
[2.897662] Unable to handle kernel NULL pointer dereference at virtual 
address 01ac
[2.897666] pgd = ff8008e0
[2.897675] [01ac] *pgd=0009e003, *pud=0009e003, 
*pmd=
[2.897680] Internal error: Oops: 9645 [#1] PREEMPT SMP
[2.897685] Modules linked in:
[2.897692] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.6.0-rc5+ #543
[2.897694] Hardware name: ARM Juno development board (r1) (DT)
[2.897699] task: ffc9768a ti: ffc9768a8000 task.ti: 
ffc9768a8000
[2.897711] PC is at __memcpy+0x7c/0x180
[2.897719] LR is at OUT_RINGp+0x34/0x70
[2.897724] pc : [] lr : [] pstate: 
8045
[2.897726] sp : ffc9768ab360
[2.897732] x29: ffc9768ab360 x28: 0001
[2.897738] x27: ffc97624c000 x26: 
[2.897744] x25: 0080 x24: 6c00
[2.897749] x23: 0005 x22: ffc97624c010
[2.897755] x21: 0004 x20: 0004
[2.897761] x19: ffc9763da000 x18: ffc976b2491c
[2.897766] x17: 0007 x16: 0006
[2.897771] x15: 0001 x14: 0001
[2.89] x13: 00e31b70 x12: ffc9768a0080
[2.897783] x11:  x10: fb00
[2.897788] x9 :  x8 : 
[2.897793] x7 :  x6 : 01ac
[2.897799] x5 :  x4 : 
[2.897804] x3 : 0010 x2 : 0010
[2.897810] x1 : ffc97624c010 x0 : 01ac
...
[2.898494] Call trace:
[2.898499] Exception stack(0xffc9768ab1a0 to 0xffc9768ab2c0)
[2.898506] b1a0: ffc9763da000 0004 ffc9768ab360 
ff80083465fc
[2.898513] b1c0: ffc976801e00 ffc9762b8000 ffc9768ab1f0 
ff80080ec158
[2.898520] b1e0: ffc9768ab230 ff8008496d04 ffc975ce6d80 
ffc9768ab36e
[2.898527] b200: ffc9768ab36f ffc9768ab29d ffc9768ab29e 
ffc9768a
[2.898533] b220: ffc9768ab250 ff80080e70c0 ffc9768ab270 
ff8008496e44
[2.898540] b240: 01ac ffc97624c010 0010 
0010
[2.898546] b260:   01ac 

[2.898552] b280:   fb00 

[2.898558] b2a0: ffc9768a0080 00e31b70 0001 
0001
[2.898566] [] __memcpy+0x7c/0x180
[2.898574] [] nv04_fbcon_imageblit+0x1d4/0x2e8
[2.898582] [] nouveau_fbcon_imageblit+0xd8/0xe0
[2.898591] [] soft_cursor+0x154/0x1d8
[2.898598] [] bit_cursor+0x4fc/0x538
[2.898605] [] fbcon_cursor+0x134/0x1a8
[2.898613] [] hide_cursor+0x38/0xa0
[2.898620] [] redraw_screen+0x120/0x228
[2.898628] [] fbcon_prepare_logo+0x370/0x3f8
[2.898635] [] fbcon_init+0x350/0x560
[2.898641] [] visual_init+0xac/0x108
[2.898648] [] do_bind_con_driver+0x1c4/0x3a8
[2.898655] [] do_take_over_console+0x174/0x1e8
[2.898662] [] do_fbcon_takeover+0x74/0x100
[2.898669] [] fbcon_event_notify+0x8cc/0x920
[2.898680] [] notifier_call_chain+0x50/0x90
[2.898685] [] __blocking_notifier_call_chain+0x4c/0x90
[2.898691] [] blocking_notifier_call_chain+0x14/0x20
[2.898696] [] fb_notifier_call_chain+0x1c/0x28
[2.898703] [] register_framebuffer+0x1cc/0x2e0
[2.898712] [] drm_fb_helper_initial_config+0x288/0x3e8
[2.898719] [] nouveau_fbcon_init+0xe0/0x118
[2.898727] [] nouveau_drm_load+0x268/0x890
[2.898734] [] drm_dev_register+0xbc/0xc8
[2.898740] [] drm_get_pci_dev+0xa0/0x180
[2.898747] [] nouveau_drm_probe+0x1a0/0x1e0
[2.898755] [] pci_device_probe+0x98/0x110
[2.898763] [] driver_probe_device+0x204/0x2b0
[2.898770] [] __driver_attach+0xac/0xb0
[2.898777] [] bus_for_each_dev+0x60/0xa0
[2.898783] [] driver_attach+0x20/0x28
[2.898789] [] bus_add_driver+0x1d0/0x238
[2.898796] [] driver_register+0x60/0xf8
[2.898802] [] __pci_register_driver+0x3c/0x48
[2.898809] [] drm_pci_init+0xf4/0x120
[2.898818] [] nouveau_drm_init+0x21c/0x230
[2.898825] [] do_one_initcall+0x8c/0x190
[2.898832

[Nouveau] [PATCH] Revert "drm/nouveau/device/pci: set as non-CPU-coherent on ARM64"

2016-05-10 Thread Robin Murphy

[2.898845] [] ret_from_fork+0x10/0x40
[2.898853] Code: a88120c7 a8c12027 a88120c7 a8c12027 (a88120c7)
[2.898871] ---[ end trace d5713dcad023ee04 ]---
[2.89] Kernel panic - not syncing: Attempted to kill init! 
exitcode=0x000b

In a toss-up between the GPU seeing stale data artefacts on some systems
vs. catastrophic kernel crashes on other systems, the latter would seem
to take precedence, so revert this change until the real underlying
problem can be fixed.

Signed-off-by: Robin Murphy 
---

Alex, Ben, Dave,

I know Alex was looking into this, but since we're nearly at -rc6 already
it looks like the only thing to do for 4.6 is pick the lesser of two evils...

Thanks,
Robin.

 drivers/gpu/drm/nouveau/nvkm/engine/device/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/pci.c 
b/drivers/gpu/drm/nouveau/nvkm/engine/device/pci.c
index 18fab3973ce5..62ad0300cfa5 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/device/pci.c
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/pci.c
@@ -1614,7 +1614,7 @@ nvkm_device_pci_func = {
.fini = nvkm_device_pci_fini,
.resource_addr = nvkm_device_pci_resource_addr,
.resource_size = nvkm_device_pci_resource_size,
-   .cpu_coherent = !IS_ENABLED(CONFIG_ARM) && !IS_ENABLED(CONFIG_ARM64),
+   .cpu_coherent = !IS_ENABLED(CONFIG_ARM),
 };
 
 int
-- 
2.8.1.dirty

___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 10/10] ACPI: IORT: Allow COMPILE_TEST of IORT

Re: [Nouveau] [PATCH 06/10] iommu: Replace iommu_device_lock with iommu_probe_device_lock

Re: [Nouveau] [PATCH v2 04/10] iommu/dma: Use the gfp parameter in __iommu_dma_alloc_noncontiguous()

Re: [Nouveau] [PATCH 1/8] iommu: Add a gfp parameter to iommu_map()

[Nouveau] [PATCH] drm/nouveau/tegra: Stop using iommu_present()

Re: [Nouveau] [PATCH 1/1] drm/nouveau/device: Get right pgsize_bitmap of iommu_domain

Re: [Nouveau] [PATCH v5 16/16] of: Add plumbing for restricted DMA pool

Re: [Nouveau] [PATCH v5 14/16] dma-direct: Allocate memory from restricted DMA pool if available

Re: [Nouveau] [PATCH v5 08/16] swiotlb: Update is_swiotlb_active to add a struct device argument

Re: [Nouveau] [PATCH RFC v1 2/6] swiotlb: convert variables to arrays

Re: [Nouveau] [PATCH 08/18] dma-mapping: add a new dma_alloc_noncoherent API

Re: [Nouveau] [trivial PATCH] treewide: Convert switch/case fallthrough; to break;

Re: [Nouveau] [PATCH 05/28] media/v4l2: remove V4L2-FLAG-MEMORY-NON-CONSISTENT

Re: [Nouveau] [PATCH 05/28] media/v4l2: remove V4L2-FLAG-MEMORY-NON-CONSISTENT

Re: [Nouveau] [PATCH 08/11] drm/nouveau: tegra: Skip IOMMU initialization if already attached

Re: [Nouveau] [PATCH 08/11] drm/nouveau: tegra: Skip IOMMU initialization if already attached

Re: DMA-API: cacheline tracking ENOMEM, dma-debug disabled due to nouveau ?

Re: [Nouveau] [RFC PATCH v1 0/6] Resolve unwanted DMA backing with IOMMU

Re: [Nouveau] [RFC PATCH v1 0/6] Resolve unwanted DMA backing with IOMMU

Re: [Nouveau] [RFC PATCH v1 0/6] Resolve unwanted DMA backing with IOMMU

Re: [Nouveau] [PATCH v4 2/2] drm/nouveau: tegra: Detach from ARM DMA/IOMMU mapping

Re: [Nouveau] [PATCH v4 1/2] ARM: dma-mapping: Set proper DMA ops in arm_iommu_detach_device()

Re: [Nouveau] [PATCH v3 2/2] drm/nouveau: tegra: Detach from ARM DMA/IOMMU mapping

Re: [Nouveau] [PATCH v3 1/2] ARM: dma-mapping: Implement arm_dma_iommu_detach_device()

Re: [Nouveau] [PATCH v3 2/2] drm/nouveau: tegra: Detach from ARM DMA/IOMMU mapping

Re: [Nouveau] [PATCH v3 2/2] drm/nouveau: tegra: Detach from ARM DMA/IOMMU mapping

Re: [Nouveau] [PATCH v3 1/2] ARM: dma-mapping: Implement arm_dma_iommu_detach_device()

Re: [Nouveau] [PATCH v2 2/5] dma-mapping: Introduce dma_iommu_detach_device() API

Re: [Nouveau] [PATCH v2 2/5] dma-mapping: Introduce dma_iommu_detach_device() API

Re: [Nouveau] [PATCH v2] Revert "drm/nouveau/device/pci: set as non-CPU-coherent on ARM64"

[Nouveau] [PATCH] Revert "drm/nouveau/device/pci: set as non-CPU-coherent on ARM64"

31 matches

Site Navigation

Mail list logo

Footer information