Re: [PATCH v4 3/6] PCI: Add support for multiple DMA aliases

[Bjorn Helgaas]

On Mon, Mar 14, 2016 at 10:43:40PM +, David Woodhouse wrote:
> On Thu, 2016-02-25 at 08:38 -0600, Bjorn Helgaas wrote:
> > 
> > >  /*
> > > - * Look for aliases to or from the given device for exisiting groups.  
> > > The
> > > - * dma_alias_devfn only supports aliases on the same bus, therefore the 
> > > search
> > > + * Look for aliases to or from the given device for existing groups. DMA
> > > + * aliases are only supported on the same bus, therefore the search
> > 
> > I'm trying to reconcile this statement that "DMA aliases are only
> > supported on the same bus" (which was there even before this patch)
> > with the fact that pci_for_each_dma_alias() does not have that
> > limitation.
> 
> Doesn't it? You can still only set a DMA alias on the same bus with
> pci_add_dma_alias(), can't you?

I guess it's true that PCI_DEV_FLAGS_DMA_ALIAS_DEVFN and the proposed
pci_add_dma_alias() only add aliases on the same bus.  I was thinking
about a scenario like this:

  00:00.0 PCIe-to-PCI bridge to [bus 01]
  01:01.0 conventional PCI device

where I think 01:00.0 is a DMA alias for 01:01.0 because the bridge
takes ownership of DMA transactions from 01:01.0 and assigns a
Requester ID of 01:00.0 (secondary bus number, device 0, function 0).

> > >   * space is quite small (especially since we're really only looking at 
> > >pcie
> > >   * device, and therefore only expect multiple slots on the root complex 
> > >or
> > >   * downstream switch ports).  It's conceivable though that a pair of
> > > @@ -686,11 +692,8 @@ static struct iommu_group 
> > > *get_pci_alias_group(struct pci_dev *pdev,
> > >   continue;
> > >  
> > >   /* We alias them or they alias us */
> > > - if (((pdev->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
> > > -  pdev->dma_alias_devfn == tmp->devfn) ||
> > > - ((tmp->dev_flags & PCI_DEV_FLAGS_DMA_ALIAS_DEVFN) &&
> > > -  tmp->dma_alias_devfn == pdev->devfn)) {
> > > -
> > > + if (dma_alias_is_enabled(pdev, tmp->devfn) ||
> > > + dma_alias_is_enabled(tmp, pdev->devfn)) {
> > >   group = get_pci_alias_group(tmp, devfns);
> > 
> > We basically have this:
> > 
> >   for_each_pci_dev(tmp) {
> >     if ()
> >   group = get_pci_alias_group();
> >   ...
> >   }
> 
> Strictly, that's:
> 
>  for_each_pci_dev(tmp) {
>    if (pdev is an alias of tmp || tmp is an alias of pdev)
>      group = get_pci_alias_group();
>    ...
>  }

OK.  

> > I'm trying to figure out why we don't do something like the following
> > instead:
> > 
> >   callback(struct pci_dev *pdev, u16 alias, void *opaque)
> >   {
> >     struct iommu_group *group;
> > 
> >     group = get_pci_alias_group();
> >     if (group)
> >   return group;
> > 
> >     return 0;
> >   }
> > 
> >   pci_for_each_dma_alias(pdev, callback, ...);
> 
> And this would be equivalent to
> 
>  for_each_pci_dev(tmp) {
>    if (tmp is an alias of pdev)
>      group = get_pci_alias_group();
>    ...
>  }
> 
> The "is an alias of" property is not commutative. Perhaps it should be.
> But that's hard because in some cases the alias doesn't even *exist* as
> a real PCI device. It's just that you appear to get DMA transactions
> from a given source-id.

Right.  In my example above, 01:00.0 is not a PCI device; it's only a
Requester ID that is fabricated by the bridge when it forwards DMA
transactions upstream.

I think I'm confused because I don't really understand IOMMU groups.

Let me explain what I think they are and you can correct me when I go
wrong.  The iommu_group_alloc() comment says "The IOMMU group
represents the minimum granularity of the IOMMU."  So I suppose the
IOMMU cannot distinguish between devices in a group.  All the devices
in the group use the same set of DMA mappings.  Granting device A DMA
access to a buffer grants the same access to all other members of A's
IOMMU group.

That would mean my question was fundamentally backwards.  In
get_pci_alias_group(A), we're not trying to figure out what all the
aliases of A are, which is what pci_for_each_dma_alias() does.

Instead, we're trying to figure out which IOMMU group A belongs to.
But I still don't quite understand how aliases fit into this.  Let's
go back to my example and assume we've already put 00:00.0 and 01:01.0
in IOMMU groups:

  00:00.0 PCIe-to-PCI bridge to [bus 01] # in IOMMU group G0
  01:01.0 conventional PCI device# in IOMMU group G1

I assume these devices are in different IOMMU groups because if the
bridge generated its own DMA, it would use Requester ID 00:00.0, which
is distinct from the 01:00.0 it would use when forwarding DMAs from
its secondary side.

What happens when we add 01:02.0?  I think 01:01.0 and 01:02.0 should
both end up in IOMMU group G1 because the IOMMU will see only
Requester ID 01:00.0, so it can't distinguish them.

When we add 01:02.0, the ops->add_device() ... ops->device_group()
path 

Re: [PATCH] iommu/vt-d: Ratelimit fault handler

[Joe Perches]

On Tue, 2016-03-15 at 10:35 -0600, Alex Williamson wrote:
> Fault rates can easily overwhelm the console and make the system
> unresponsive.  Ratelimit to allow an opportunity for maintenance.

A few suggestions:

o Use a single ratelimit state.
o The multiple lines output are unnecessary and hard to grep
  in the dmesg output because of inconsistent prefixing as
  second and subsequent output lines are not prefixed by pr_fmt.
o The DMAR prefix on the second block is also unnecessary as
  it's already prefixed by pr_fmt
o Coalesce the formats for easier grep.

so maybe:
---
 drivers/iommu/dmar.c | 28 
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index 8ffd756..59dcaaa 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -1575,23 +1575,27 @@ static int dmar_fault_do_one(struct intel_iommu *iommu, 
int type,
 {
    const char *reason;
    int fault_type;
+   static DEFINE_RATELIMIT_STATE(rs,
+     DEFAULT_RATELIMIT_INTERVAL,
+     DEFAULT_RATELIMIT_BURST);
+
+   if (__ratelimit())
+   return 0;
 
    reason = dmar_get_fault_reason(fault_reason, _type);
 
    if (fault_type == INTR_REMAP)
-   pr_err("INTR-REMAP: Request device [[%02x:%02x.%d] "
-      "fault index %llx\n"
-   "INTR-REMAP:[fault reason %02d] %s\n",
-   (source_id >> 8), PCI_SLOT(source_id & 0xFF),
-   PCI_FUNC(source_id & 0xFF), addr >> 48,
-   fault_reason, reason);
+   pr_err("[INTR-REMAP] Request device [%02x:%02x.%d] fault index 
%llx [fault reason %02d] %s\n",
+      source_id >> 8, PCI_SLOT(source_id & 0xFF),
+      PCI_FUNC(source_id & 0xFF), addr >> 48,
+      fault_reason, reason);
    else
-   pr_err("DMAR:[%s] Request device [%02x:%02x.%d] "
-      "fault addr %llx \n"
-      "DMAR:[fault reason %02d] %s\n",
-      (type ? "DMA Read" : "DMA Write"),
-      (source_id >> 8), PCI_SLOT(source_id & 0xFF),
-      PCI_FUNC(source_id & 0xFF), addr, fault_reason, reason);
+   pr_err("[%s] Request device [%02x:%02x.%d] fault addr %llx 
[fault reason %02d] %s\n",
+      type ? "DMA Read" : "DMA Write",
+      source_id >> 8, PCI_SLOT(source_id & 0xFF),
+      PCI_FUNC(source_id & 0xFF), addr,
+      fault_reason, reason);
+
    return 0;
 }
 

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

Re: [PATCH] iommu/vt-d: Ratelimit fault handler

[David Woodhouse]

On Tue, 2016-03-15 at 10:35 -0600, Alex Williamson wrote:
> Fault rates can easily overwhelm the console and make the system
> unresponsive.  Ratelimit to allow an opportunity for maintenance.
> 
> Signed-off-by: Alex Williamson 

Rather than just rate-limiting the printk, I'd prefer to handle this
explicitly. There's a bit in the context-entry which can tell the IOMMU
not to bother raising an interrupt at all. And then we can re-enable it
if/when the driver recovers the device. (Or perhaps just when it next
does a mapping).

We really ought to be reporting faults to drivers too, FWIW. I keep
meaning to take a look at that.

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 0/7] Intel IOMMU scalability improvements

[Ben Serebrin via iommu]

There are nice.  Thanks very much for doing this work!

We have some preliminary results, looking at scaling to high core counts.  
We tested the patches on a 2-socket high core count SNB-EP server with a 
Broadcomm NIC.  Our benchmark uses 200 threads of TCP_RR.  We see similar 
performance for IOMMU disabled as we do for IOMMU enabled with this patchset, 
which is good news.  We're working on getting a lab setup with Haswell servers 
so we can further test the scalability of the code.

We owe the scaling results and of course actual code reviews.



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

Re: [PATCH] iommu/vt-d: Ratelimit fault handler

[Joe Perches]

On Tue, 2016-03-15 at 10:10 -0700, Joe Perches wrote:
> o Use a single ratelimit state.
[]
> diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
[]
> + if (__ratelimit())
> + return 0;

That of course should be:

if (!__ratelimit())
return 0;
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 01/04] iommu/ipmmu-vmsa: 32-bit ARM may have CONFIG_IOMMU_DMA=y

[Magnus Damm]

From: Magnus Damm 

Instead of assuming that CONFIG_ARM=y also means CONFIG_IOMMU_DMA=n,
convert the #ifdefs to take CONFIG_IOMMU_DMA into consideration
so 32-bit ARM can make use of CONFIG_IOMMU_DMA=y as well once those
bits are in place.

Signed-off-by: Magnus Damm 
---

 drivers/iommu/ipmmu-vmsa.c |6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

--- 0007/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c 2016-03-15 21:05:40.590513000 +0900
@@ -22,7 +22,7 @@
 #include 
 #include 
 
-#ifdef CONFIG_ARM
+#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
 #include 
 #include 
 #endif
@@ -40,7 +40,7 @@ struct ipmmu_vmsa_device {
DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
 
-#ifdef CONFIG_ARM
+#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
struct dma_iommu_mapping *mapping;
 #endif
 };
@@ -619,7 +619,7 @@ static int ipmmu_find_utlbs(struct ipmmu
return 0;
 }
 
-#ifdef CONFIG_ARM
+#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
 static int ipmmu_map_attach(struct device *dev, struct ipmmu_vmsa_device *mmu)
 {
int ret;
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 04/04] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops

[Magnus Damm]

From: Magnus Damm 

Introduce a new set of iommu_ops suitable for 64-bit ARM
as well as 32-bit ARM when CONFIG_IOMMU_DMA=y. The ->of_xlate()
callback is needed by the code exported by of_iommu.h and
it is wrapped in #ifdefs to also compile of x86_64.

Signed-off-by: Magnus Damm 
---

 drivers/iommu/ipmmu-vmsa.c |   94 +++-
 1 file changed, 92 insertions(+), 2 deletions(-)

--- 0011/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c 2016-03-16 01:35:06.990513000 +0900
@@ -10,6 +10,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -804,7 +805,7 @@ static void ipmmu_remove_device(struct d
dev->archdata.iommu = NULL;
 }
 
-static const struct iommu_ops ipmmu_ops = {
+static const struct iommu_ops __maybe_unused ipmmu_ops = {
.domain_alloc = ipmmu_domain_alloc,
.domain_free = ipmmu_domain_free,
.attach_dev = ipmmu_attach_device,
@@ -818,6 +819,92 @@ static const struct iommu_ops ipmmu_ops
.pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K,
 };
 
+static struct iommu_domain *ipmmu_domain_alloc_dma(unsigned type)
+{
+   struct iommu_domain *io_domain;
+
+   if (type != IOMMU_DOMAIN_DMA)
+   return NULL;
+
+   io_domain = __ipmmu_domain_alloc(type);
+   if (io_domain)
+   iommu_get_dma_cookie(io_domain);
+
+   return io_domain;
+}
+
+static void ipmmu_domain_free_dma(struct iommu_domain *io_domain)
+{
+   iommu_put_dma_cookie(io_domain);
+   ipmmu_domain_free(io_domain);
+}
+
+static int ipmmu_add_device_dma(struct device *dev)
+{
+   struct iommu_group *group;
+
+   /* only accept devices with iommus property */
+   if (of_count_phandle_with_args(dev->of_node, "iommus",
+  "#iommu-cells") < 0)
+   return -ENODEV;
+
+   group = iommu_group_get_for_dev(dev);
+   if (IS_ERR(group))
+   return PTR_ERR(group);
+
+   return 0;
+}
+
+static void ipmmu_remove_device_dma(struct device *dev)
+{
+   iommu_group_remove_device(dev);
+}
+
+static struct iommu_group *ipmmu_device_group_dma(struct device *dev)
+{
+   struct iommu_group *group;
+   int ret;
+
+   group = generic_device_group(dev);
+   if (IS_ERR(group))
+   return group;
+
+   ret = ipmmu_init_platform_device(dev, group);
+   if (ret) {
+   iommu_group_put(group);
+   group = ERR_PTR(ret);
+   }
+
+   return group;
+}
+
+#ifdef CONFIG_OF_IOMMU
+static int ipmmu_of_xlate_dma(struct device *dev,
+ struct of_phandle_args *spec)
+{
+   /* dummy callback to satisfy of_iommu_configure() */
+   return 0;
+}
+#endif
+
+static const struct iommu_ops __maybe_unused ipmmu_ops_dma = {
+   .domain_alloc = ipmmu_domain_alloc_dma,
+   .domain_free = ipmmu_domain_free_dma,
+   .attach_dev = ipmmu_attach_device,
+   .detach_dev = ipmmu_detach_device,
+   .map = ipmmu_map,
+   .unmap = ipmmu_unmap,
+   .map_sg = default_iommu_map_sg,
+   .iova_to_phys = ipmmu_iova_to_phys,
+   .add_device = ipmmu_add_device_dma,
+   .remove_device = ipmmu_remove_device_dma,
+   .device_group = ipmmu_device_group_dma,
+   .pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K,
+#ifdef CONFIG_OF_IOMMU
+   .of_xlate = ipmmu_of_xlate_dma,
+#endif
+};
+
 /* 
-
  * Probe/remove and init
  */
@@ -929,14 +1016,17 @@ static struct platform_driver ipmmu_driv
 
 static int __init ipmmu_init(void)
 {
+   const struct iommu_ops *ops;
int ret;
 
ret = platform_driver_register(_driver);
if (ret < 0)
return ret;
 
+   ops = IS_ENABLED(CONFIG_IOMMU_DMA) ? _ops_dma : _ops;
+
if (!iommu_present(_bus_type))
-   bus_set_iommu(_bus_type, _ops);
+   bus_set_iommu(_bus_type, ops);
 
return 0;
 }
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 02/04] iommu/ipmmu-vmsa: Break out utlb parsing code

[Magnus Damm]

From: Magnus Damm 

Break out the utlb parsing code and dev_data allocation into a
separate function. This is preparation for future code sharing.

Signed-off-by: Magnus Damm 
---

 drivers/iommu/ipmmu-vmsa.c |  125 
 1 file changed, 70 insertions(+), 55 deletions(-)

--- 0008/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c 2016-03-15 21:08:20.180513000 +0900
@@ -619,6 +619,69 @@ static int ipmmu_find_utlbs(struct ipmmu
return 0;
 }
 
+static int ipmmu_init_platform_device(struct device *dev,
+ struct iommu_group *group)
+{
+   struct ipmmu_vmsa_archdata *archdata;
+   struct ipmmu_vmsa_device *mmu;
+   unsigned int *utlbs;
+   unsigned int i;
+   int num_utlbs;
+   int ret = -ENODEV;
+
+   /* Find the master corresponding to the device. */
+
+   num_utlbs = of_count_phandle_with_args(dev->of_node, "iommus",
+  "#iommu-cells");
+   if (num_utlbs < 0)
+   return -ENODEV;
+
+   utlbs = kcalloc(num_utlbs, sizeof(*utlbs), GFP_KERNEL);
+   if (!utlbs)
+   return -ENOMEM;
+
+   spin_lock(_devices_lock);
+
+   list_for_each_entry(mmu, _devices, list) {
+   ret = ipmmu_find_utlbs(mmu, dev, utlbs, num_utlbs);
+   if (!ret) {
+   /*
+* TODO Take a reference to the MMU to protect
+* against device removal.
+*/
+   break;
+   }
+   }
+
+   spin_unlock(_devices_lock);
+
+   if (ret < 0)
+   goto error;
+
+   for (i = 0; i < num_utlbs; ++i) {
+   if (utlbs[i] >= mmu->num_utlbs) {
+   ret = -EINVAL;
+   goto error;
+   }
+   }
+
+   archdata = kzalloc(sizeof(*archdata), GFP_KERNEL);
+   if (!archdata) {
+   ret = -ENOMEM;
+   goto error;
+   }
+
+   archdata->mmu = mmu;
+   archdata->utlbs = utlbs;
+   archdata->num_utlbs = num_utlbs;
+   dev->archdata.iommu = archdata;
+   return 0;
+
+error:
+   kfree(utlbs);
+   return ret;
+}
+
 #if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
 static int ipmmu_map_attach(struct device *dev, struct ipmmu_vmsa_device *mmu)
 {
@@ -675,13 +738,8 @@ static inline void ipmmu_release_mapping
 
 static int ipmmu_add_device(struct device *dev)
 {
-   struct ipmmu_vmsa_archdata *archdata;
-   struct ipmmu_vmsa_device *mmu;
-   struct iommu_group *group = NULL;
-   unsigned int *utlbs;
-   unsigned int i;
-   int num_utlbs;
-   int ret = -ENODEV;
+   struct iommu_group *group;
+   int ret;
 
if (dev->archdata.iommu) {
dev_warn(dev, "IOMMU driver already assigned to device %s\n",
@@ -689,42 +747,6 @@ static int ipmmu_add_device(struct devic
return -EINVAL;
}
 
-   /* Find the master corresponding to the device. */
-
-   num_utlbs = of_count_phandle_with_args(dev->of_node, "iommus",
-  "#iommu-cells");
-   if (num_utlbs < 0)
-   return -ENODEV;
-
-   utlbs = kcalloc(num_utlbs, sizeof(*utlbs), GFP_KERNEL);
-   if (!utlbs)
-   return -ENOMEM;
-
-   spin_lock(_devices_lock);
-
-   list_for_each_entry(mmu, _devices, list) {
-   ret = ipmmu_find_utlbs(mmu, dev, utlbs, num_utlbs);
-   if (!ret) {
-   /*
-* TODO Take a reference to the MMU to protect
-* against device removal.
-*/
-   break;
-   }
-   }
-
-   spin_unlock(_devices_lock);
-
-   if (ret < 0)
-   return -ENODEV;
-
-   for (i = 0; i < num_utlbs; ++i) {
-   if (utlbs[i] >= mmu->num_utlbs) {
-   ret = -EINVAL;
-   goto error;
-   }
-   }
-
/* Create a device group and add the device to it. */
group = iommu_group_alloc();
if (IS_ERR(group)) {
@@ -742,27 +764,20 @@ static int ipmmu_add_device(struct devic
goto error;
}
 
-   archdata = kzalloc(sizeof(*archdata), GFP_KERNEL);
-   if (!archdata) {
-   ret = -ENOMEM;
+   ret = ipmmu_init_platform_device(dev, group);
+   if (ret < 0) {
+   dev_err(dev, "Failed to init platform device\n");
goto error;
}
 
-   archdata->mmu = mmu;
-   archdata->utlbs = utlbs;
-   archdata->num_utlbs = num_utlbs;
-   dev->archdata.iommu = archdata;
-
-   ret = ipmmu_map_attach(dev, mmu);
+   ret = ipmmu_map_attach(dev, ((struct ipmmu_vmsa_archdata *)
+ 

[PATCH 00/04] iommu/ipmmu-vmsa: IPMMU CONFIG_IOMMU_DMA update

[Magnus Damm]

iommu/ipmmu-vmsa: IPMMU CONFIG_IOMMU_DMA update

[PATCH 01/04] iommu/ipmmu-vmsa: 32-bit ARM may have CONFIG_IOMMU_DMA=y
[PATCH 02/04] iommu/ipmmu-vmsa: Break out utlb parsing code
[PATCH 03/04] iommu/ipmmu-vmsa: Break out domain allocation code
[PATCH 04/04] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops

This series updates the IPMMU driver with CONFIG_IOMMU_DMA=y support
which is present in mainline for 64-bit ARM while experimental support
has been posted for 32-bit ARM thanks to Marek Szyprowski:

[RFC 0/3] Unify IOMMU-based DMA-mapping code for ARM and ARM64

Most of the patches in this series based on code earlier included in
the series below but has been reworked to also fit on 32-bit ARM:

[PATCH/RFC 00/10] iommu/ipmmu-vmsa: Experimental r8a7795 support

Signed-off-by: Magnus Damm <damm+rene...@opensource.se>
---

 Built on top of next-20160315
 Depends on [PATCH v2 00/04] iommu/ipmmu-vmsa: IPMMU multi-arch update V2

 drivers/iommu/ipmmu-vmsa.c |  238 
 1 file changed, 174 insertions(+), 64 deletions(-)

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

[PATCH] iommu/vt-d: Ratelimit fault handler

[Alex Williamson]

Fault rates can easily overwhelm the console and make the system
unresponsive.  Ratelimit to allow an opportunity for maintenance.

Signed-off-by: Alex Williamson 
---
 drivers/iommu/dmar.c |   28 +++-
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index 8ffd756..628987e 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -1579,19 +1579,20 @@ static int dmar_fault_do_one(struct intel_iommu *iommu, 
int type,
reason = dmar_get_fault_reason(fault_reason, _type);
 
if (fault_type == INTR_REMAP)
-   pr_err("INTR-REMAP: Request device [[%02x:%02x.%d] "
-  "fault index %llx\n"
-   "INTR-REMAP:[fault reason %02d] %s\n",
-   (source_id >> 8), PCI_SLOT(source_id & 0xFF),
-   PCI_FUNC(source_id & 0xFF), addr >> 48,
-   fault_reason, reason);
+   pr_err_ratelimited("INTR-REMAP: Request device [[%02x:%02x.%d] "
+  "fault index %llx\n"
+  "INTR-REMAP:[fault reason %02d] %s\n",
+  (source_id >> 8), PCI_SLOT(source_id & 0xFF),
+  PCI_FUNC(source_id & 0xFF), addr >> 48,
+  fault_reason, reason);
else
-   pr_err("DMAR:[%s] Request device [%02x:%02x.%d] "
-  "fault addr %llx \n"
-  "DMAR:[fault reason %02d] %s\n",
-  (type ? "DMA Read" : "DMA Write"),
-  (source_id >> 8), PCI_SLOT(source_id & 0xFF),
-  PCI_FUNC(source_id & 0xFF), addr, fault_reason, reason);
+   pr_err_ratelimited("DMAR:[%s] Request device [%02x:%02x.%d] "
+  "fault addr %llx \n"
+  "DMAR:[fault reason %02d] %s\n",
+  (type ? "DMA Read" : "DMA Write"),
+  (source_id >> 8), PCI_SLOT(source_id & 0xFF),
+  PCI_FUNC(source_id & 0xFF), addr,
+  fault_reason, reason);
return 0;
 }
 
@@ -1606,7 +1607,8 @@ irqreturn_t dmar_fault(int irq, void *dev_id)
raw_spin_lock_irqsave(>register_lock, flag);
fault_status = readl(iommu->reg + DMAR_FSTS_REG);
if (fault_status)
-   pr_err("DRHD: handling fault status reg %x\n", fault_status);
+   pr_err_ratelimited("DRHD: handling fault status reg %x\n",
+  fault_status);
 
/* TBD: ignore advanced fault log currently */
if (!(fault_status & DMA_FSTS_PPF))

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

Re: [RFC 3/3] iommu: dma-iommu: use common implementation also on ARM architecture

[Robin Murphy]

Hi Marek, Arnd,

On 19/02/16 10:30, Arnd Bergmann wrote:

On Friday 19 February 2016 09:22:44 Marek Szyprowski wrote:

This patch replaces ARM-specific IOMMU-based DMA-mapping implementation
with generic IOMMU DMA-mapping code shared with ARM64 architecture. The
side-effect of this change is a switch from bitmap-based IO address space
management to tree-based code. There should be no functional changes
for drivers, which rely on initialization from generic arch_setup_dna_ops()
interface. Code, which used old arm_iommu_* functions must be updated to
new interface.

Signed-off-by: Marek Szyprowski 


I like the overall idea. However, this interface from the iommu
subsystem into architecture specific code:


+/*
+ * The DMA API is built upon the notion of "buffer ownership".  A buffer
+ * is either exclusively owned by the CPU (and therefore may be accessed
+ * by it) or exclusively owned by the DMA device.  These helper functions
+ * represent the transitions between these two ownership states.
+ *
+ * Note, however, that on later ARMs, this notion does not work due to
+ * speculative prefetches.  We model our approach on the assumption that
+ * the CPU does do speculative prefetches, which means we clean caches
+ * before transfers and delay cache invalidation until transfer completion.
+ *
+ */
+extern void __dma_page_cpu_to_dev(struct page *, unsigned long, size_t,
+ enum dma_data_direction);
+extern void __dma_page_dev_to_cpu(struct page *, unsigned long, size_t,
+ enum dma_data_direction);
+
+static inline void arch_flush_page(struct device *dev, const void *virt,
+   phys_addr_t phys)
+{
+   dmac_flush_range(virt, virt + PAGE_SIZE);
+   outer_flush_range(phys, phys + PAGE_SIZE);
+}
+
+static inline void arch_dma_map_area(phys_addr_t phys, size_t size,
+enum dma_data_direction dir)
+{
+   unsigned int offset = phys & ~PAGE_MASK;
+   __dma_page_cpu_to_dev(phys_to_page(phys & PAGE_MASK), offset, size, 
dir);
+}
+
+static inline void arch_dma_unmap_area(phys_addr_t phys, size_t size,
+  enum dma_data_direction dir)
+{
+   unsigned int offset = phys & ~PAGE_MASK;
+   __dma_page_dev_to_cpu(phys_to_page(phys & PAGE_MASK), offset, size, 
dir);
+}
+
+static inline pgprot_t arch_get_dma_pgprot(struct dma_attrs *attrs,
+   pgprot_t prot, bool coherent)
+{
+   if (coherent)
+   return prot;
+
+   prot = dma_get_attr(DMA_ATTR_WRITE_COMBINE, attrs) ?
+   pgprot_writecombine(prot) :
+   pgprot_dmacoherent(prot);
+   return prot;
+}
+
+extern void *arch_alloc_from_atomic_pool(size_t size, struct page **ret_page,
+gfp_t flags);
+extern bool arch_in_atomic_pool(void *start, size_t size);
+extern int arch_free_from_atomic_pool(void *start, size_t size);
+
+


doesn't feel completely right yet. In particular the arch_flush_page()
interface is probably still too specific to ARM/ARM64 and won't work
that way on other architectures.

I think it would be better to do this either more generic, or less generic:

a) leave the iommu_dma_map_ops definition in the architecture specific
code, but make it call helper functions in the drivers/iommu to do all
of the really generic parts.


This was certainly the original intent of the arm64 code. The division 
of responsibility there is a conscious decision - IOMMU-API-wrangling 
goes in the common code, cache maintenance and actual dma_map_ops stay 
hidden in architecture-private code, safe from abuse. It's very much 
modelled on SWIOTLB.


Given all the work Russell did last year getting rid of direct uses of 
the dmac_* cache maintenance functions by ARM drivers, I don't think 
bringing all of that back is a good way to go - Personally I'd much 
rather see several dozen lines of very similar looking (other than 
highmem and outer cache stuff) arch-private code if it maintains a 
robust and clearly-defined abstraction (and avoids yet another level of 
indirection). It does also seem a little odd to factor out only half the 
file on the grounds of architectural similarity, when that argument 
applies equally to the other (non-IOMMU) half too. I think the recent 
tree-wide conversion to generic dma_map_ops was in part motivated by the 
thought of common implementations, so I'm sure that's something we can 
revisit in due course.


Robin.



b) clarify that this is only applicable to arch/arm and arch/arm64, and
unify things further between these two, as they have very similar
requirements in the CPU architecture.

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

Re: [RFC 3/3] iommu: dma-iommu: use common implementation also on ARM architecture

[Marek Szyprowski]

Hello,

On 2016-03-15 12:18, Magnus Damm wrote:

Hi Marek,

On Fri, Feb 19, 2016 at 5:22 PM, Marek Szyprowski
 wrote:

This patch replaces ARM-specific IOMMU-based DMA-mapping implementation
with generic IOMMU DMA-mapping code shared with ARM64 architecture. The
side-effect of this change is a switch from bitmap-based IO address space
management to tree-based code. There should be no functional changes
for drivers, which rely on initialization from generic arch_setup_dna_ops()
interface. Code, which used old arm_iommu_* functions must be updated to
new interface.

Signed-off-by: Marek Szyprowski 
---

Thanks for your efforts and my apologies for late comments. Just FYI
I'll try your patch (and this series) with the ipmmu-vmsa.c driver on
32-bit ARM and see how it goes. Nice not to have to support multiple
interfaces depending on architecture!


Thanks for testing!


One question that comes to mind is how to handle features.

For instance, the 32-bit ARM code supports DMA_ATTR_FORCE_CONTIGUOUS
while the shared code in drivers/iommu/dma-iommu.c does not. I assume
existing users may rely on such features so from my point of view it
probably makes sense to carry over features from the 32-bit ARM code
into the shared code before pulling the plug.


Right, this has to be added to common code before merging.


I also wonder if it is possible to do a step-by-step migration and
support both old and new interfaces in the same binary? That may make
things easier for multiplatform enablement. So far I've managed to
make one IOMMU driver support both 32-bit ARM and 64-bit ARM with some
ugly magic, so adjusting 32-bit ARM dma-mapping code to coexist with
the shared code in drivers/iommu/dma-iommu.c may also be possible. And
probably involving even more ugly magic. =)


Having one IOMMU driver for both 32-bit and 64-bit ARM archs is quite easy
IF you rely on the iommu core to setup everything. See exynos-iommu driver
- after my last patches it now works fine on both archs (using arch
specific interfaces). Most of the magic is done automatically by
arch_setup_dma_ops().

The real problem is the fact that there are drivers (like DRM) which rely
on specific dma-mapping functions from ARM architecture, which need to be
rewritten.

Best regards
--
Marek Szyprowski, PhD
Samsung R Institute Poland

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

Re: [RFC 3/3] iommu: dma-iommu: use common implementation also on ARM architecture

[Robin Murphy]

Hi Magnus,

On 15/03/16 11:18, Magnus Damm wrote:

Hi Marek,

On Fri, Feb 19, 2016 at 5:22 PM, Marek Szyprowski
 wrote:

This patch replaces ARM-specific IOMMU-based DMA-mapping implementation
with generic IOMMU DMA-mapping code shared with ARM64 architecture. The
side-effect of this change is a switch from bitmap-based IO address space
management to tree-based code. There should be no functional changes
for drivers, which rely on initialization from generic arch_setup_dna_ops()
interface. Code, which used old arm_iommu_* functions must be updated to
new interface.

Signed-off-by: Marek Szyprowski 
---


Thanks for your efforts and my apologies for late comments. Just FYI
I'll try your patch (and this series) with the ipmmu-vmsa.c driver on
32-bit ARM and see how it goes. Nice not to have to support multiple
interfaces depending on architecture!

One question that comes to mind is how to handle features.

For instance, the 32-bit ARM code supports DMA_ATTR_FORCE_CONTIGUOUS
while the shared code in drivers/iommu/dma-iommu.c does not. I assume
existing users may rely on such features so from my point of view it
probably makes sense to carry over features from the 32-bit ARM code
into the shared code before pulling the plug.


Indeed - the patch I posted the other day doing proper scatterlist 
merging in the common code is largely to that end.



I also wonder if it is possible to do a step-by-step migration and
support both old and new interfaces in the same binary? That may make
things easier for multiplatform enablement. So far I've managed to
make one IOMMU driver support both 32-bit ARM and 64-bit ARM with some
ugly magic, so adjusting 32-bit ARM dma-mapping code to coexist with
the shared code in drivers/iommu/dma-iommu.c may also be possible. And
probably involving even more ugly magic. =)


That was also my thought when I tried to look at this a while ago - I 
started on some patches moving the bitmap from dma_iommu_mapping into 
the iommu_domain->iova_cookie so that the existing code and users could 
then be converted to just passing iommu_domains around, after which it 
should be fairly painless to swap out the back-end implementation 
transparently. That particular effort ground to a halt upon realising 
the number of the IOMMU and DRM drivers I'd have no way of testing - if 
you're interested I've dug out the diff below from an old 
work-in-progress branch (which probably doesn't even compile).


Robin.



Cheers,

/ magnus


--->8---
diff --git a/arch/arm/include/asm/device.h b/arch/arm/include/asm/device.h
index 4111592..6ea939c 100644
--- a/arch/arm/include/asm/device.h
+++ b/arch/arm/include/asm/device.h
@@ -14,9 +14,6 @@ struct dev_archdata {
 #ifdef CONFIG_IOMMU_API
void *iommu; /* private IOMMU data */
 #endif
-#ifdef CONFIG_ARM_DMA_USE_IOMMU
-   struct dma_iommu_mapping*mapping;
-#endif
bool dma_coherent;
 };

@@ -28,10 +25,4 @@ struct pdev_archdata {
 #endif
 };

-#ifdef CONFIG_ARM_DMA_USE_IOMMU
-#define to_dma_iommu_mapping(dev) ((dev)->archdata.mapping)
-#else
-#define to_dma_iommu_mapping(dev) NULL
-#endif
-
 #endif
diff --git a/arch/arm/include/asm/dma-iommu.h 
b/arch/arm/include/asm/dma-iommu.h

index 2ef282f..e15197d 100644
--- a/arch/arm/include/asm/dma-iommu.h
+++ b/arch/arm/include/asm/dma-iommu.h
@@ -24,13 +24,12 @@ struct dma_iommu_mapping {
struct kref kref;
 };

-struct dma_iommu_mapping *
+struct iommu_domain *
 arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, u64 size);

-void arm_iommu_release_mapping(struct dma_iommu_mapping *mapping);
+void arm_iommu_release_mapping(struct iommu_domain *mapping);

-int arm_iommu_attach_device(struct device *dev,
-   struct dma_iommu_mapping *mapping);
+int arm_iommu_attach_device(struct device *dev, struct iommu_domain 
*mapping);

 void arm_iommu_detach_device(struct device *dev);

 #endif /* __KERNEL__ */
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index e62400e..dfb5001 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1246,7 +1246,8 @@ __iommu_alloc_remap(struct page **pages, size_t 
size, gfp_t gfp, pgprot_t prot,

 static dma_addr_t
 __iommu_create_mapping(struct device *dev, struct page **pages, size_t 
size)

 {
-   struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
+   struct iommu_domain *dom = iommu_get_domain_for_dev(dev);
+   struct dma_iommu_mapping *mapping = dom->iova_cookie;
unsigned int count = PAGE_ALIGN(size) >> PAGE_SHIFT;
dma_addr_t dma_addr, iova;
int i;
@@ -1268,8 +1269,7 @@ __iommu_create_mapping(struct device *dev, struct 
page **pages, size_t size)

break;

len = (j - i) << PAGE_SHIFT;
-   ret = iommu_map(mapping->domain, iova, phys, len,
-   IOMMU_READ|IOMMU_WRITE);
+ 

Re: [RFC 3/3] iommu: dma-iommu: use common implementation also on ARM architecture

[Magnus Damm]

Hi Marek,

On Fri, Feb 19, 2016 at 5:22 PM, Marek Szyprowski
 wrote:
> This patch replaces ARM-specific IOMMU-based DMA-mapping implementation
> with generic IOMMU DMA-mapping code shared with ARM64 architecture. The
> side-effect of this change is a switch from bitmap-based IO address space
> management to tree-based code. There should be no functional changes
> for drivers, which rely on initialization from generic arch_setup_dna_ops()
> interface. Code, which used old arm_iommu_* functions must be updated to
> new interface.
>
> Signed-off-by: Marek Szyprowski 
> ---

Thanks for your efforts and my apologies for late comments. Just FYI
I'll try your patch (and this series) with the ipmmu-vmsa.c driver on
32-bit ARM and see how it goes. Nice not to have to support multiple
interfaces depending on architecture!

One question that comes to mind is how to handle features.

For instance, the 32-bit ARM code supports DMA_ATTR_FORCE_CONTIGUOUS
while the shared code in drivers/iommu/dma-iommu.c does not. I assume
existing users may rely on such features so from my point of view it
probably makes sense to carry over features from the 32-bit ARM code
into the shared code before pulling the plug.

I also wonder if it is possible to do a step-by-step migration and
support both old and new interfaces in the same binary? That may make
things easier for multiplatform enablement. So far I've managed to
make one IOMMU driver support both 32-bit ARM and 64-bit ARM with some
ugly magic, so adjusting 32-bit ARM dma-mapping code to coexist with
the shared code in drivers/iommu/dma-iommu.c may also be possible. And
probably involving even more ugly magic. =)

Cheers,

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

Re: [PATCH V5 02/10] perf/amd/iommu: Consolidate and move perf_event_amd_iommu header

[Peter Zijlstra]

On Tue, Mar 15, 2016 at 11:40:17AM +0100, Borislav Petkov wrote:
> On Tue, Mar 15, 2016 at 07:39:31AM +0700, Suravee Suthikulpanit wrote:
> > What if I just merge the newly introduced arch/x86/include/perf/amd/iommu.h
> > into the include/linux/amd-iommu.h? I do not see the point of having to
> > separate things out into two files.
> 
> Except that this header has x86-specific stuff and include/linux/ is
> arch-agnostic.

Which would suggest that header is placed wrong, because I would expect
the amd-iommu to really be rather x86 specific. Or is AMD making ARM
parts for which this is useful too?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH V5 02/10] perf/amd/iommu: Consolidate and move perf_event_amd_iommu header

[Borislav Petkov]

On Tue, Mar 15, 2016 at 07:39:31AM +0700, Suravee Suthikulpanit wrote:
> What if I just merge the newly introduced arch/x86/include/perf/amd/iommu.h
> into the include/linux/amd-iommu.h? I do not see the point of having to
> separate things out into two files.

Except that this header has x86-specific stuff and include/linux/ is
arch-agnostic.

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v5 3/9] dma-mapping: add dma_{map,unmap}_resource

[Christoph Hellwig]

On Fri, Mar 11, 2016 at 01:58:46PM +0100, Niklas S?derlund wrote:
> Without an IOMMU this is easy since the phys_addr_t and dma_addr_t are 
> the same and no special care is needed. However if you have a IOMMU you 
> need to map the DMA slave phys_addr_t to a dma_addr_t using something 
> like this. Is it not very similar to dma_map_single() where one maps 
> processor virtual memory (instead if MMIO) so that it can be used with 
> DMA slaves?

It's similar, but I don't think this actually works as a general case
as there are quite a few places that expect to be able to have a
struct page for a physical address.  We'd at least need a very careful
audit for that case.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH V5 02/10] perf/amd/iommu: Consolidate and move perf_event_amd_iommu header

[Peter Zijlstra]

On Tue, Mar 15, 2016 at 07:39:31AM +0700, Suravee Suthikulpanit wrote:
> What if I just merge the newly introduced arch/x86/include/perf/amd/iommu.h
> into the include/linux/amd-iommu.h? I do not see the point of having to
> separate things out into two files.
> 

Works for me. Thanks!
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu