Re: [PATCH 2/5] iommu: Set PCI_BUS_FLAGS_MSI_REMAP if IOMMU have capability of IRQ remapping

[Yongji Xie]

On 2016/5/25 5:11, Bjorn Helgaas wrote:


On Wed, Apr 27, 2016 at 08:43:27PM +0800, Yongji Xie wrote:

The capability of IRQ remapping is abstracted on IOMMU side on
some archs. There is a existing flag IOMMU_CAP_INTR_REMAP for this.

To have a universal flag to test this capability for different
archs on PCI side, we set PCI_BUS_FLAGS_MSI_REMAP for PCI buses
when IOMMU_CAP_INTR_REMAP is set.

Signed-off-by: Yongji Xie 
---
  drivers/iommu/iommu.c |   15 +++
  1 file changed, 15 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 0e3b009..5d2b6f6 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -813,6 +813,16 @@ struct iommu_group *pci_device_group(struct device *dev)
return group;
  }
  
+static void pci_check_msi_remapping(struct pci_dev *pdev,

+   const struct iommu_ops *ops)
+{
+   struct pci_bus *bus = pdev->bus;
+
+   if (ops->capable(IOMMU_CAP_INTR_REMAP) &&
+   !(bus->bus_flags & PCI_BUS_FLAGS_MSI_REMAP))
+   bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP;
+}

This looks an awful lot like the pci_bus_check_msi_remapping() you add
elsewhere.  Why do we need both?


I will modify this function as you suggested.  And we add this function
here because some iommu drivers would be initialed after PCI probing.


  /**
   * iommu_group_get_for_dev - Find or create the IOMMU group for a device
   * @dev: target device
@@ -871,6 +881,9 @@ static int add_iommu_group(struct device *dev, void *data)
const struct iommu_ops *ops = cb->ops;
int ret;
  
+	if (dev_is_pci(dev) && ops->capable)

+   pci_check_msi_remapping(to_pci_dev(dev), ops);
+
if (!ops->add_device)
return 0;
  
@@ -913,6 +926,8 @@ static int iommu_bus_notifier(struct notifier_block *nb,

 * result in ADD/DEL notifiers to group->notifier
 */
if (action == BUS_NOTIFY_ADD_DEVICE) {
+   if (dev_is_pci(dev) && ops->capable)
+   pci_check_msi_remapping(to_pci_dev(dev), ops);

These calls don't smell right either.  Why do we need dev_is_pci()
checks here?


Some platform devices may also call this.


Can't this be done in the PCI probe path somehow, e.g.,
in pci_set_bus_msi_domain() or something?



Yes, this can be done in pci_create_root_bus(). But it could only
handle the case that iommu drivers are initialed before PCI probing.

Regards,
Yongji

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

Re: [PATCH] iommu/vt-d: reduce extra first level entry in iommu->domains

[Wei Yang]

Hi, Joerg

Not sure whether you think this calculation is correct.

If I missed something for this " + 1" in your formula, I am glad to hear your
explanation. So that I could learn something from you :-)

Have a good day~

On Sat, May 21, 2016 at 02:41:51AM +, Wei Yang wrote:
>In commit <8bf478163e69> ("iommu/vt-d: Split up iommu->domains array"), it
>it splits iommu->domains in two levels. Each first level contains 256
>entries of second level. In case of the ndomains is exact a multiple of
>256, it would have one more extra first level entry for current
>implementation.
>
>This patch refines this calculation to reduce the extra first level entry.
>
>Signed-off-by: Wei Yang 
>---
> drivers/iommu/intel-iommu.c |4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
>index e3061d3..2204ca4 100644
>--- a/drivers/iommu/intel-iommu.c
>+++ b/drivers/iommu/intel-iommu.c
>@@ -1634,7 +1634,7 @@ static int iommu_init_domains(struct intel_iommu *iommu)
>   return -ENOMEM;
>   }
> 
>-  size = ((ndomains >> 8) + 1) * sizeof(struct dmar_domain **);
>+  size = (ALIGN(ndomains, 256) >> 8) * sizeof(struct dmar_domain **);
>   iommu->domains = kzalloc(size, GFP_KERNEL);
> 
>   if (iommu->domains) {
>@@ -1699,7 +1699,7 @@ static void disable_dmar_iommu(struct intel_iommu *iommu)
> static void free_dmar_iommu(struct intel_iommu *iommu)
> {
>   if ((iommu->domains) && (iommu->domain_ids)) {
>-  int elems = (cap_ndoms(iommu->cap) >> 8) + 1;
>+  int elems = ALIGN(cap_ndoms(iommu->cap), 256) >> 8;
>   int i;
> 
>   for (i = 0; i < elems; i++)
>-- 
>1.7.9.5

-- 
Wei Yang
Help you, Help me
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 2/5] iommu: Set PCI_BUS_FLAGS_MSI_REMAP if IOMMU have capability of IRQ remapping

[Bjorn Helgaas]

On Wed, Apr 27, 2016 at 08:43:27PM +0800, Yongji Xie wrote:
> The capability of IRQ remapping is abstracted on IOMMU side on
> some archs. There is a existing flag IOMMU_CAP_INTR_REMAP for this.
> 
> To have a universal flag to test this capability for different
> archs on PCI side, we set PCI_BUS_FLAGS_MSI_REMAP for PCI buses
> when IOMMU_CAP_INTR_REMAP is set.
> 
> Signed-off-by: Yongji Xie 
> ---
>  drivers/iommu/iommu.c |   15 +++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 0e3b009..5d2b6f6 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -813,6 +813,16 @@ struct iommu_group *pci_device_group(struct device *dev)
>   return group;
>  }
>  
> +static void pci_check_msi_remapping(struct pci_dev *pdev,
> + const struct iommu_ops *ops)
> +{
> + struct pci_bus *bus = pdev->bus;
> +
> + if (ops->capable(IOMMU_CAP_INTR_REMAP) &&
> + !(bus->bus_flags & PCI_BUS_FLAGS_MSI_REMAP))
> + bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP;
> +}

This looks an awful lot like the pci_bus_check_msi_remapping() you add
elsewhere.  Why do we need both?

>  /**
>   * iommu_group_get_for_dev - Find or create the IOMMU group for a device
>   * @dev: target device
> @@ -871,6 +881,9 @@ static int add_iommu_group(struct device *dev, void *data)
>   const struct iommu_ops *ops = cb->ops;
>   int ret;
>  
> + if (dev_is_pci(dev) && ops->capable)
> + pci_check_msi_remapping(to_pci_dev(dev), ops);
> +
>   if (!ops->add_device)
>   return 0;
>  
> @@ -913,6 +926,8 @@ static int iommu_bus_notifier(struct notifier_block *nb,
>* result in ADD/DEL notifiers to group->notifier
>*/
>   if (action == BUS_NOTIFY_ADD_DEVICE) {
> + if (dev_is_pci(dev) && ops->capable)
> + pci_check_msi_remapping(to_pci_dev(dev), ops);

These calls don't smell right either.  Why do we need dev_is_pci()
checks here?  Can't this be done in the PCI probe path somehow, e.g.,
in pci_set_bus_msi_domain() or something?

>   if (ops->add_device)
>   return ops->add_device(dev);
>   } else if (action == BUS_NOTIFY_REMOVED_DEVICE) {
> -- 
> 1.7.9.5
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/5] PCI: Set PCI_BUS_FLAGS_MSI_REMAP if MSI controller supports IRQ remapping

[Bjorn Helgaas]

On Wed, Apr 27, 2016 at 08:43:28PM +0800, Yongji Xie wrote:
> On ARM HW the capability of IRQ remapping is abstracted on
> MSI controller side. MSI_FLAG_IRQ_REMAPPING is used to advertise
> this [1].
> 
> To have a universal flag to test this capability for different
> archs on PCI side, we set PCI_BUS_FLAGS_MSI_REMAP for PCI buses
> when MSI_FLAG_IRQ_REMAPPING is set.
> 
> [1] http://www.spinics.net/lists/kvm/msg130256.html
> 
> Signed-off-by: Yongji Xie 
> ---
>  drivers/pci/msi.c   |   12 
>  drivers/pci/probe.c |3 +++
>  include/linux/msi.h |6 +-
>  3 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index a080f44..1661cdf 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1134,6 +1134,18 @@ void *msi_desc_to_pci_sysdata(struct msi_desc *desc)
>  }
>  EXPORT_SYMBOL_GPL(msi_desc_to_pci_sysdata);
>  
> +void pci_bus_check_msi_remapping(struct pci_bus *bus,
> +  struct irq_domain *domain)
> +{
> +#ifdef CONFIG_PCI_MSI_IRQ_DOMAIN
> + struct msi_domain_info *info;
> +
> + info = msi_get_domain_info(domain);
> + if (info->flags & MSI_FLAG_IRQ_REMAPPING)
> + bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP;
> +#endif
> +}

Functions named "check_foo" are a pet peeve of mine because the name
doesn't tell us anything about what the function *does*.  In this
case, we know it checks something about MSI remapping, but we don't
know whether we're checking whether it's enabled, disabled, or some
other property.

I'd prefer something like:

  int pci_bus_msi_isolated(struct pci_bus *bus, struct irq_domain *domain)
  {
struct msi_domain_info *info;

if (!domain)
  return 0;

info = msi_get_domain_info(domain);
if (info->flags & MSI_FLAG_IRQ_REMAPPING)
  return 1;

return 0;
  }

  void pci_set_bus_msi_domain(struct pci_bus *bus)
  {
...
if (b == bus && pci_bus_msi_isolated(bus, d))
  bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP;

>  #ifdef CONFIG_PCI_MSI_IRQ_DOMAIN
>  /**
>   * pci_msi_domain_write_msg - Helper to write MSI message to PCI config space
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 6d7ab9b..25cf1b1 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -696,6 +696,9 @@ static void pci_set_bus_msi_domain(struct pci_bus *bus)
>   if (!d)
>   d = pci_host_bridge_msi_domain(b);
>  
> + if (d && b == bus)
> + pci_bus_check_msi_remapping(bus, d);
> +
>   dev_set_msi_domain(>dev, d);
>  }
>  
> diff --git a/include/linux/msi.h b/include/linux/msi.h
> index 03eda72..b4c649e 100644
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -15,6 +15,8 @@ extern int pci_msi_ignore_mask;
>  struct irq_data;
>  struct msi_desc;
>  struct pci_dev;
> +struct pci_bus;
> +struct irq_domain;
>  struct platform_msi_priv_data;
>  void __get_cached_msi_msg(struct msi_desc *entry, struct msi_msg *msg);
>  void get_cached_msi_msg(unsigned int irq, struct msi_msg *msg);
> @@ -155,6 +157,9 @@ void arch_restore_msi_irqs(struct pci_dev *dev);
>  void default_teardown_msi_irqs(struct pci_dev *dev);
>  void default_restore_msi_irqs(struct pci_dev *dev);
>  
> +void pci_bus_check_msi_remapping(struct pci_bus *bus,
> +  struct irq_domain *domain);
> +
>  struct msi_controller {
>   struct module *owner;
>   struct device *dev;
> @@ -173,7 +178,6 @@ struct msi_controller {
>  #include 
>  #include 
>  
> -struct irq_domain;
>  struct irq_domain_ops;
>  struct irq_chip;
>  struct device_node;
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/5] PCI: Add a new PCI_BUS_FLAGS_MSI_REMAP flag

[Bjorn Helgaas]

On Wed, Apr 27, 2016 at 08:43:26PM +0800, Yongji Xie wrote:
> We introduce a new pci_bus_flags, PCI_BUS_FLAGS_MSI_REMAP
> which indicates all devices on the bus are protected by the
> hardware which supports IRQ remapping(intel naming).

This changelog is ambiguous.  It's possible that there is hardware
that *supports* IRQ remapping, but does not actually *do* IRQ
remapping.  For example, an IRQ remapping capability may be present
but not enabled.

I think your intent is to set this flag only when MSI remapping is
actually *enabled* for all devices on the bus.

I'd also like to know exactly what protection is implied by
PCI_BUS_FLAGS_MSI_REMAP and IOMMU_CAP_INTR_REMAP.  I guess it means a
device can only generate MSIs to a certain set of CPUs?  I assume the
remapping hardware only checks the target address, not the data being
written?

> This flag will be used to know whether it's safe to expose
> MSI-X tables of PCI BARs to userspace. Because the capability
> of IRQ remapping can guarantee the PCI device cannot trigger
> MSIs that correspond to interrupt IDs of other devices.
> 
> There is a existing flag for this in the IOMMU space:
> 
> enum iommu_cap {
>   IOMMU_CAP_CACHE_COHERENCY,
> --->  IOMMU_CAP_INTR_REMAP,
>   IOMMU_CAP_NOEXEC,
> };
> 
> and Eric also posted a patchset [1] to abstract this
> capability on MSI controller side for ARM. But it would
> make sense to have a more common flag like
> PCI_BUS_FLAGS_MSI_REMAP in this patch so that we can use
> a universal flag to test this capability on PCI side for
> different archs.
> 
> [1] http://www.spinics.net/lists/kvm/msg130256.html
> 
> Signed-off-by: Yongji Xie 
> ---
>  include/linux/pci.h |1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 27df4a6..d619228 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -193,6 +193,7 @@ typedef unsigned short __bitwise pci_bus_flags_t;
>  enum pci_bus_flags {
>   PCI_BUS_FLAGS_NO_MSI   = (__force pci_bus_flags_t) 1,
>   PCI_BUS_FLAGS_NO_MMRBC = (__force pci_bus_flags_t) 2,
> + PCI_BUS_FLAGS_MSI_REMAP = (__force pci_bus_flags_t) 4,
>  };
>  
>  /* These values come from the PCI Express Spec */
> -- 
> 1.7.9.5
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC 8/9] drivers: of: call iommu_bus_add_dev after iommu_configure_ops

[Robin Murphy]

On 25/04/16 16:58, Sricharan R wrote:

Now that the device's iommu ops are configured at probe time,
the device has to be added to the iommu late.

Signed-off-by: Sricharan R 
---
  drivers/of/device.c | 4 
  1 file changed, 4 insertions(+)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 57a5f2d..722115c 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -6,6 +6,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -154,6 +155,9 @@ int of_dma_configure_ops(struct device *dev, struct 
device_node *np)
dev_dbg(dev, "device is%sbehind an iommu\n",
iommu ? " " : " not ");

+   if (iommu)
+   iommu_bus_add_dev(dev);


This (in conjunction with the previous patch) seems unnecessarily 
convoluted - if of_iommu_configure() has found some iommu_ops for a 
device, why not just call .add_device() directly there and then? There 
are already systems that could warrant having two different IOMMU 
drivers active simultaneously (but thankfully don't _need_ to), so 
trying to escape from per-bus IOMMU ops makes more sense than 
entrenching the horrible notion of "the" IOMMU on "the" platform bus any 
further.


Robin.


+
arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);

return 0;



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

Re: [RFC 5/9] drivers: platform: Configure dma operations at probe time

[Robin Murphy]

On 25/04/16 16:58, Sricharan R wrote:

From: Laurent Pinchart 

Configuring DMA ops at probe time will allow deferring device probe when
the IOMMU isn't available yet.

Signed-off-by: Laurent Pinchart 
---
  drivers/base/platform.c | 13 +
  drivers/of/platform.c   |  7 +++
  2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index f437afa..17e64a4 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -557,6 +557,12 @@ static int platform_drv_probe(struct device *_dev)
if (ret < 0)
return ret;

+   if (of_have_populated_dt()) {
+   ret = of_dma_configure_ops(_dev, _dev->of_node);
+   if (ret < 0)
+   goto done;
+   }
+
ret = dev_pm_domain_attach(_dev, true);
if (ret != -EPROBE_DEFER) {
if (drv->probe) {
@@ -569,6 +575,11 @@ static int platform_drv_probe(struct device *_dev)
}
}

+   if (of_have_populated_dt()) {
+   if (ret)
+   of_dma_deconfigure(_dev);
+   }
+done:
if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
dev_warn(_dev, "probe deferral not supported\n");
ret = -ENXIO;
@@ -591,6 +602,8 @@ static int platform_drv_remove(struct device *_dev)
if (drv->remove)
ret = drv->remove(dev);
dev_pm_domain_detach(_dev, true);
+   if (of_have_populated_dt())
+   of_dma_deconfigure(_dev);

return ret;
  }
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 17ee8d5..12bbc8e1 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -180,11 +180,9 @@ static struct platform_device 
*of_platform_device_create_pdata(
dev->dev.bus = _bus_type;
dev->dev.platform_data = platform_data;
of_dma_configure_masks(>dev, dev->dev.of_node);
-   of_dma_configure_ops(>dev, dev->dev.of_node);
of_msi_configure(>dev, dev->dev.of_node);

if (of_device_add(dev) != 0) {
-   of_dma_deconfigure(>dev);
platform_device_put(dev);
goto err_clear_flag;
}
@@ -481,11 +479,12 @@ static int of_platform_device_destroy(struct device *dev, 
void *data)
if (dev->bus == _bus_type)
platform_device_unregister(to_platform_device(dev));
  #ifdef CONFIG_ARM_AMBA
-   else if (dev->bus == _bustype)
+   else if (dev->bus == _bustype) {
amba_device_unregister(to_amba_device(dev));
+   of_dma_deconfigure(dev);
+   }


Note that we definitely need this to happen properly on other buses, 
too. I had something for the AMBA bus back when I last looked at 
this[0], but since then we now have PCI devices going through the 
of_dma_configure() path as well (hence 226d89cbb242).


There's also now some initial ACPI code in flight[1], so it's probably 
about time to take another closer look at how this can be properly 
generalised, but I'm not necessarily averse to starting with a 
DT-focused solution that gets at least half way, then building on top of 
that.


Robin.

[0]:http://www.linux-arm.org/git?p=linux-rm.git;a=commitdiff;h=1f38b62b05e17ccbfcc68e4229c69fee74922e2a
[1]:http://thread.gmane.org/gmane.linux.kernel.iommu/13046


  #endif

-   of_dma_deconfigure(dev);
of_node_clear_flag(dev->of_node, OF_POPULATED);
of_node_clear_flag(dev->of_node, OF_POPULATED_BUS);
return 0;



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

Re: [PATCH v2 4/5] iommu/mediatek: add support for mtk iommu generation one HW

[Robin Murphy]

On 24/05/16 10:57, Honghui Zhang wrote:
[...]

@@ -48,6 +48,9 @@ struct mtk_iommu_domain {
struct io_pgtable_ops   *iop;

struct iommu_domain domain;
+   void*pgt_va;
+   dma_addr_t  pgt_pa;
+   void*cookie;


These are going to be mutually exclusive with the cfg and iop members,
which implies it might be a good idea to use a union and not waste
space. Or better, just forward-declare struct mtk_iommu_domain here and
leave separate definitions private to each driver. The void *cookie is
also an unnecessary level of abstraction, I think.



Do you mean declare struct mtk_iommu_domain here, and implement a new
struct in mtk_iommu_v1.c like
struct mtk_iommu_domain_v1 {
struct mtk_iommu_domain domain;
u32 *pgt_va;
dma_addr_t  pgt_pa;
mtk_iommu_data  *data;
};
If this is acceptable I would implement it in the next version.


Pretty much, except they both want to be called struct mtk_iommu_domain, 
so that a *declaration* for the sake of the m4u_dom member of struct 
mtk_iommu_data in the header file can remain common to both drivers - it 
then just picks up whichever private *definition* from the .c file being 
compiled.



   };

   struct mtk_iommu_data {
diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
new file mode 100644
index 000..55023e1
--- /dev/null
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -0,0 +1,742 @@
+/*
+ * Copyright (c) 2015-2016 MediaTek Inc.
+ * Author: Yong Wu 


Nit: is that in the sense that this patch should also have Yong's
signed-off-by on it, or in that it's your work derived from his version
in mtk_iommu.c?


I write this driver based on Yong's version of mtk_iommu.c, should I add
his signed-off-by for this patch? Or should I put a comment about this?
Thanks.


OK, in that case I think the appropriate attribution would be along the 
lines of "Author: Honghui Zhang, based on mtk_iommu.c by Yong Wu" (if in 
doubt, grepping for "Based on" gives a feel for how this is commonly 
done). If the work that comprises this patch itself (i.e. the copying 
and modification of the existing code) is all yours then your sign-off 
alone is fine.


[...]

+static int mtk_iommu_add_device(struct device *dev)
+{
+   struct iommu_group *group;
+   struct device_node *np;
+   struct of_phandle_args iommu_spec;
+   int idx = 0;
+
+   while (!of_parse_phandle_with_args(dev->of_node, "iommus",
+  "#iommu-cells", idx,
+  _spec)) {


Hang on, this doesn't seem right - why do you need to reimplement all
this instead of using IOMMU_OF_DECLARE()?


All the clients of mtk generation one iommu share the same iommu domain,
as a matter of fact, mtk generation one iommu only support one iommu
domain. ALl the clients share the same iova address and use the same
pagetable. That means all iommu clients needed to be attached to the
same dma_iommu_mapping.


Ugh, right - I'd forgotten that the arch/arm DMA mapping code doesn't 
respect IOMMU groups or default domains at all. That's the real root 
cause of the issue here.



If use IOMMU_OF_DELCARE, we need to call of_iommu_set_ops to set the
iommu_ops, I do not want the iommu_ops be set since it would cause iommu
client device in different dma_iommu_mapping.

When an iommu client device has been created, the following sequence is
called.

of_platform_device_create
->of_dma_config
->arch_setup_dma_ops
->arch_setup_iommu_dma_ops
In this function of arch_setup_iommu_dma_ops would create a new
dma_iommu_mapping for each iommu client device and then attach the
device to this new dma_iommu_mapping. Since all the iommu clients share
the very same pagetable, this will not workable for our HW.
I could not release the dma_iommu_mapping in attach_device since the
to_dma_iommu_mapping was set after device_attached.
Any suggest for this?


On a second look, you're doing more or less the same thing that the 
Renesas IPMMU driver currently does, so it's probably OK as a workaround 
for now. Fixing the arch/arm code is part of the bigger ongoing problem 
of sorting out IOMMU probing and DMA configuration, and it doesn't seem 
fair to force that on you for the sake of one driver ;)


[...]

+static int __maybe_unused mtk_iommu_resume(struct device *dev)
+{
+   struct mtk_iommu_data *data = dev_get_drvdata(dev);
+   struct mtk_iommu_suspend_reg *reg = >reg;
+   void __iomem *base = data->base;
+
+   writel_relaxed(data->m4u_dom->pgt_pa, base + REG_MMU_PT_BASE_ADDR);


Hmm, this looks like the only case where m4u_dom actually seems
necessary - I'm pretty sure all the others could be fairly easily
reworked to not use it (I might try having a quick hack at the existing
M4U driver to see) - at which point we 

Re: [RFC PATCH v1 10/18] x86/efi: Access EFI related tables in the clear

[Tom Lendacky]

On 05/12/2016 01:20 PM, Tom Lendacky wrote:
> On 05/10/2016 08:57 AM, Borislav Petkov wrote:
>> On Tue, May 10, 2016 at 02:43:58PM +0100, Matt Fleming wrote:
>>> Is it not possible to maintain some kind of kernel virtual address
>>> mapping so memremap*() and friends can figure out when to twiddle the
>>> mapping attributes and map with/without encryption?
>>
>> I guess we can move the sme_* specific stuff one indirection layer
>> below, i.e., in the *memremap() routines so that callers don't have to
>> care... That should keep the churn down...
>>
> 
> We could do that, but we'll have to generate that list of addresses so
> that it can be checked against the range being mapped.  Since this is
> part of early memmap support searching that list every time might not be
> too bad. I'll have to look into that and see what that looks like.

I looked into this and this would be a large change also to parse tables
and build lists.  It occurred to me that this could all be taken care of
if the early_memremap calls were changed to early_ioremap calls. Looking
in the git log I see that they were originally early_ioremap calls but
were changed to early_memremap calls with this commit:

commit abc93f8eb6e4 ("efi: Use early_mem*() instead of early_io*()")

Looking at the early_memremap code and the early_ioremap code they both
call __early_ioremap so I don't see how this change makes any
difference (especially since FIXMAP_PAGE_NORMAL and FIXMAP_PAGE_IO are
identical in this case).

Is it safe to change these back to early_ioremap calls (at least on
x86)?

Thanks,
Tom

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

Re: [PATCH 2/4] ARM: dma-mapping: Constify attrs passed to internal functions

[Russell King - ARM Linux]

On Tue, May 24, 2016 at 08:28:08AM +0200, Krzysztof Kozlowski wrote:
> Some of the non-exported functions do not modify passed dma_attrs so the
> pointer can point to const data.
> 
> Signed-off-by: Krzysztof Kozlowski 

Acked-by: Russell King 

Thanks.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH V5 6/7] iommu/msm: Use writel_relaxed and add a barrier

[Arnd Bergmann]

On Monday, May 23, 2016 11:35:04 AM CEST Sricharan wrote:
> Hi Arnd,
> 
> >> @@ -124,6 +124,9 @@ static void msm_iommu_reset(void __iomem *base, int 
> >> ncb)
> >>SET_TLBLKCR(base, ctx, 0);
> >>SET_CONTEXTIDR(base, ctx, 0);
> >>}
> >> +
> >> +  /* Ensure completion of relaxed writes from the above SET macros */
> >> +  mb();
> >>  }
> >
> >Why do the above use the relaxed writes? Do you have any performance
> >numbers showing that skipping the sync for the reset makes a measurable
> >difference?
> >
> >How did you prove that skipping the sync before the write is safe?
> >
> >How about resetting the iommu less often instead?
> >
> 
> I had measured the numbers only for the full usecase path, not for the
> reset path alone. I saw improvement of about 5% on full numbers.
> As you said, the reset path would be called only less often
> and might not bring a measurable change. I did not see a difference in 
> behavior
> when changing the sync to happen after the writes.

Ok, then better not change it.

> But my understanding was that
> the sync after the writes was required to ensure write completion. 

Can you cite the relevant documentation on this? Is this specific
to the Qualcomm CPU implementation or the IOMMU? I don't think
the ARM architecture requires anything like this in general.

> I should have made smaller patches to do this change.
> The only patch relevant for this series is the one that changes the write in 
> _iotlb_range
> function. Rest of the changes, should be added one by one in a separate 
> series.

If you see the same 5% performance improvement with a simpler change, then
better do only that. The IOMMU infrastructure is rather sensitive to
having correct barriers everywhere, so this minimizes the risk of getting
it wrong somewhere.


> >> @@ -181,7 +187,8 @@ fail:
> >>
> >>  static void __flush_iotlb_sync(void *cookie)
> >>  {
> >> -  /* To avoid a null function pointer */
> >> +  /* To ensure completion of the TLBIVA in __flush_iotlb_range */
> >> +  mb();
> >>  }
> >
> >I don't understand the specific race from the comment.
> >
> >What operation comes after this that relies on __flush_iotlb_range
> >having completed, and how does an mb() guarantee this?
> >
> 
> The flush_iotlb_range operation invalidates the tlb for writes to
> pagetable and the finally calls the sync operation to ensure completion
> of the flush and this is required before returning back to the client
> of the iommu. In the case of this iommu, only a barrier is required to
> ensure completion of the invalidate operation. 

This doesn't answer my question: What operation would a client do
that requires the flush to be completed here? A barrier is always
defined in terms of things that come before it in combination with
things that come after it.

Any operation that could trigger a DMA from a device is required
to have a barrier preceding it (usually wmb() one implied by writel()),
so this is clearly not about a driver that installs a DMA mapping
before starting a DMA, but I don't see what else it would be.

> >This seems to be a bug fix that is unrelated to the change to
> >use writel_relaxed(), so better split it out into a separate
> >patch, with a longer changeset description. Did you spot this
> >race by running into incorrect data, or by inspection?
> >
> 
> No i did not see a data corruption issue without the mb(),
> but that it would have been hidden in someother way as well.
> Another difference was the sync  was done before the write
> previously and now its moved after the write. As i understand
> sync after the write is correct. So i will change this patch with more
> description and move rest of that changes out.

Ok.

> >> @@ -500,7 +516,8 @@ static phys_addr_t msm_iommu_iova_to_phys(struct 
> >> iommu_domain *domain,
> >>/* Invalidate context TLB */
> >>SET_CTX_TLBIALL(iommu->base, master->num, 0);
> >>SET_V2PPR(iommu->base, master->num, va & V2Pxx_VA);
> >> -
> >> +  /* Ensure completion of relaxed writes from the above SET macros */
> >> +  mb();
> >>par = GET_PAR(iommu->base, master->num);
> >>
> >>/* We are dealing with a supersection */
> >
> >In this case, I'd say it's better to rewrite the function to avoid the
> >read: iova_to_phys() should be fast, and not require hardware access.
> >Getting rid of the hardware access by using an in-memory cache for
> >this should gain more than just removing the barriers, as an MMIO read
> >is always slow
> 
> Ok, you mean using the software walk through ? I will check on this to measure
>  the latency difference. If thats true, then the iopgtable ops itself 
> provides a
> function for iova_to_phys conversion, so that can be used.

I hadn't realized that this is a lookup in the hardware, rather than
reading a static register. It's probably a good idea to check this
anyway.


Arnd
___
iommu mailing list
iommu@lists.linux-foundation.org

Re: [PATCH 4/5] iommu/rockchip: add ARM64 cache flush operation for iommu

[Catalin Marinas]

On Tue, May 24, 2016 at 10:31:17AM +0800, Shunqian Zheng wrote:
> On 2016年05月23日 21:35, Catalin Marinas wrote:
> >On Mon, May 23, 2016 at 11:44:14AM +0100, Robin Murphy wrote:
> >>On 23/05/16 02:37, Shunqian Zheng wrote:
> >>>From: Simon 
> >>>
> >>>Signed-off-by: Simon 
> >>>---
> >>>  drivers/iommu/rockchip-iommu.c | 4 
> >>>  1 file changed, 4 insertions(+)
> >>>
> >>>diff --git a/drivers/iommu/rockchip-iommu.c 
> >>>b/drivers/iommu/rockchip-iommu.c
> >>>index 043d18c..1741b65 100644
> >>>--- a/drivers/iommu/rockchip-iommu.c
> >>>+++ b/drivers/iommu/rockchip-iommu.c
> >>>@@ -95,12 +95,16 @@ struct rk_iommu {
> >>>
> >>>  static inline void rk_table_flush(u32 *va, unsigned int count)
> >>>  {
> >>>+#if defined(CONFIG_ARM)
> >>>   phys_addr_t pa_start = virt_to_phys(va);
> >>>   phys_addr_t pa_end = virt_to_phys(va + count);
> >>>   size_t size = pa_end - pa_start;
> >>>
> >>>   __cpuc_flush_dcache_area(va, size);
> >>>   outer_flush_range(pa_start, pa_end);
> >>>+#elif defined(CONFIG_ARM64)
> >>>+  __dma_flush_range(va, va + count);
> >>>+#endif
> >>Ugh, please don't use arch-private cache maintenance functions directly from
> >>a driver. Allocating/mapping page tables to be read by the IOMMU is still
> >>DMA, so using the DMA APIs is the correct way to manage them, *especially*
> >>if it needs to work across multiple architectures.
> 
> It's easier for us if changing  the __dma_flush_range() to
> __flush_dcache_area() is acceptable here?

It's not really acceptable for arm64, nor for arm32. Please fix this
driver in a similar way to commit e3c971960fd4 ("iommu/tegra-smmu:
Convert to use DMA API").

The only place where we allowed __flush_dcache_area() is in the GICv3
driver and that's because it hasn't been wired as a platform device
(yet).

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

Re: [PATCH v2 4/5] iommu/mediatek: add support for mtk iommu generation one HW

[Honghui Zhang]

Hi, Robin,
Thanks very much for your comments.

On Mon, 2016-05-23 at 20:31 +0100, Robin Murphy wrote:
> On 19/05/16 12:49, honghui.zh...@mediatek.com wrote:
> > From: Honghui Zhang 
> >
> > Mediatek SoC's M4U has two generations of HW architcture. Generation one
> > uses flat, one layer pagetable, and was shipped with ARM architecture, it
> > only supports 4K size page mapping. MT2701 SoC uses this generation one
> > m4u HW. Generation two uses the ARM short-descriptor translation table
> > format for address translation, and was shipped with ARM64 architecture,
> > MT8173 uses this generation two m4u HW. All the two generation iommu HW
> > only have one iommu domain, and all its iommu clients share the same
> > iova address.
> >
> > These two generation m4u HW have slit different register groups and
> > register offset, but most register names are the same. This patch add iommu
> > support for mediatek SoC mt2701.
> >
> > Signed-off-by: Honghui Zhang 
> > ---
> >   drivers/iommu/Kconfig|  19 ++
> >   drivers/iommu/Makefile   |   1 +
> >   drivers/iommu/mtk_iommu.h|   3 +
> >   drivers/iommu/mtk_iommu_v1.c | 742 
> > +++
> >   4 files changed, 765 insertions(+)
> >   create mode 100644 drivers/iommu/mtk_iommu_v1.c
> >
> > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> > index dd1dc39..2e17d70 100644
> > --- a/drivers/iommu/Kconfig
> > +++ b/drivers/iommu/Kconfig
> > @@ -354,4 +354,23 @@ config MTK_IOMMU
> >
> >   If unsure, say N here.
> >
> > +config MTK_IOMMU_V1
> > +   bool "MTK IOMMU Version 1 (M4U gen1) Support"
> > +   depends on ARM || ARM64
> 
> The commit message states that gen1 shipped in 32-bit hardware, and 
> 64-bit hardware has gen2, which implies that ARM64 here is unnecessary - 
> does any SoC with 64it-capable CPUs and a gen1 IOMMU actually exist?

Thanks, there's no any SoC with ARM64 CPUs and gen1 IOMMU for now, I
will remove the un-necessary dependency.

> 
> > +   depends on ARCH_MEDIATEK || COMPILE_TEST
> > +   select ARM_DMA_USE_IOMMU
> > +   select IOMMU_API
> > +   select IOMMU_DMA
> 
> Either way you don't need this - arm64 already selects it as necessary, 
> and it's not used on 32-bit.

Thanks.

> 
> > +   select MEMORY
> > +   select MTK_SMI
> > +   select COMMON_CLK_MT2701_MMSYS
> > +   select COMMON_CLK_MT2701_IMGSYS
> > +   select COMMON_CLK_MT2701_VDECSYS
> > +   help
> > + Support for the M4U on certain Mediatek SoCs. M4U generation 1 HW is
> > + Multimedia Memory Managememt Unit. This option enables remapping of
> > + DMA memory accesses for the multimedia subsystem.
> > +
> > + if unsure, say N here.
> > +
> >   endif # IOMMU_SUPPORT
> > diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
> > index c6edb31..778baf5 100644
> > --- a/drivers/iommu/Makefile
> > +++ b/drivers/iommu/Makefile
> > @@ -18,6 +18,7 @@ obj-$(CONFIG_INTEL_IOMMU_SVM) += intel-svm.o
> >   obj-$(CONFIG_IPMMU_VMSA) += ipmmu-vmsa.o
> >   obj-$(CONFIG_IRQ_REMAP) += intel_irq_remapping.o irq_remapping.o
> >   obj-$(CONFIG_MTK_IOMMU) += mtk_iommu.o
> > +obj-$(CONFIG_MTK_IOMMU_V1) += mtk_iommu_v1.o
> >   obj-$(CONFIG_OMAP_IOMMU) += omap-iommu.o
> >   obj-$(CONFIG_OMAP_IOMMU_DEBUG) += omap-iommu-debug.o
> >   obj-$(CONFIG_ROCKCHIP_IOMMU) += rockchip-iommu.o
> > diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
> > index 5656355..8d60f21 100644
> > --- a/drivers/iommu/mtk_iommu.h
> > +++ b/drivers/iommu/mtk_iommu.h
> > @@ -48,6 +48,9 @@ struct mtk_iommu_domain {
> > struct io_pgtable_ops   *iop;
> >
> > struct iommu_domain domain;
> > +   void*pgt_va;
> > +   dma_addr_t  pgt_pa;
> > +   void*cookie;
> 
> These are going to be mutually exclusive with the cfg and iop members, 
> which implies it might be a good idea to use a union and not waste 
> space. Or better, just forward-declare struct mtk_iommu_domain here and 
> leave separate definitions private to each driver. The void *cookie is 
> also an unnecessary level of abstraction, I think.
> 

Do you mean declare struct mtk_iommu_domain here, and implement a new
struct in mtk_iommu_v1.c like
struct mtk_iommu_domain_v1 {
struct mtk_iommu_domain domain;
u32 *pgt_va;
dma_addr_t  pgt_pa;
mtk_iommu_data  *data;
};
If this is acceptable I would implement it in the next version.

> >   };
> >
> >   struct mtk_iommu_data {
> > diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
> > new file mode 100644
> > index 000..55023e1
> > --- /dev/null
> > +++ b/drivers/iommu/mtk_iommu_v1.c
> > @@ -0,0 +1,742 @@
> > +/*
> > + * Copyright (c) 2015-2016 MediaTek Inc.
> > + * Author: Yong Wu 
> 
> Nit: is that in the sense that this patch should also have Yong's 
> signed-off-by on it, or 

Re: [PATCH 0/4] dma-mapping: Constify dma_attrs

[Christoph Hellwig]

I think this is moving into the wrong direction.  The right fix here
is to get of all the dma_attrs boilerplate code and just replace it
with a simple enum dma_flags.  This would simplify both the callers
and most importantly the wrappers for the flag-less versions a lot.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 1/4] dma-mapping: Constify attrs passed to dma_get_attr

[Krzysztof Kozlowski]

The dma_get_attr() does not modify passed dma_attrs so the pointer can
point to const data.

Signed-off-by: Krzysztof Kozlowski 
---
 include/linux/dma-attrs.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/dma-attrs.h b/include/linux/dma-attrs.h
index 5246239a4953..f3c5aeadb100 100644
--- a/include/linux/dma-attrs.h
+++ b/include/linux/dma-attrs.h
@@ -60,7 +60,8 @@ static inline void dma_set_attr(enum dma_attr attr, struct 
dma_attrs *attrs)
  * @attr: attribute to set
  * @attrs: struct dma_attrs (may be NULL)
  */
-static inline int dma_get_attr(enum dma_attr attr, struct dma_attrs *attrs)
+static inline int dma_get_attr(enum dma_attr attr,
+  const struct dma_attrs *attrs)
 {
if (attrs == NULL)
return 0;
-- 
1.9.1

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

[PATCH 3/4] arm64: dma-mapping: Constify attrs passed to internal functions

[Krzysztof Kozlowski]

Some of the non-exported functions do not modify passed dma_attrs so the
pointer can point to const data.

Signed-off-by: Krzysztof Kozlowski 
---
 arch/arm64/mm/dma-mapping.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index c566ec83719f..0ef620a34c4e 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -29,7 +29,7 @@
 
 #include 
 
-static pgprot_t __get_dma_pgprot(struct dma_attrs *attrs, pgprot_t prot,
+static pgprot_t __get_dma_pgprot(const struct dma_attrs *attrs, pgprot_t prot,
 bool coherent)
 {
if (!coherent || dma_get_attr(DMA_ATTR_WRITE_COMBINE, attrs))
@@ -88,7 +88,7 @@ static int __free_from_pool(void *start, size_t size)
 
 static void *__dma_alloc_coherent(struct device *dev, size_t size,
  dma_addr_t *dma_handle, gfp_t flags,
- struct dma_attrs *attrs)
+ const struct dma_attrs *attrs)
 {
if (dev == NULL) {
WARN_ONCE(1, "Use an actual device structure for DMA 
allocation\n");
@@ -118,7 +118,7 @@ static void *__dma_alloc_coherent(struct device *dev, 
size_t size,
 
 static void __dma_free_coherent(struct device *dev, size_t size,
void *vaddr, dma_addr_t dma_handle,
-   struct dma_attrs *attrs)
+   const struct dma_attrs *attrs)
 {
bool freed;
phys_addr_t paddr = dma_to_phys(dev, dma_handle);
-- 
1.9.1

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

[RFC 4/4] dma-mapping: Constify dma_attrs

[Krzysztof Kozlowski]

Pointer to dma_attrs passed to all dma-mapping implementations can point
to const data. This brings some benefits:
 - const-safeness,
 - is a direct indication that ownership of memory is not transferred to
   called functions so it can be safely allocated on the stack (which is
   a pattern already used).

Please have in mind that this is RFC, not finished yet. Only ARM and
ARM64 are fixed. However other API users also have to be converted which
is quite intrusive. I would rather avoid it until the overall approach
is accepted.

Signed-off-by: Krzysztof Kozlowski 
---
 arch/arm/include/asm/dma-mapping.h | 12 
 arch/arm/mm/dma-mapping.c  | 61 +-
 arch/arm/xen/mm.c  |  4 +--
 arch/arm64/mm/dma-mapping.c| 47 +++--
 drivers/iommu/dma-iommu.c  |  6 ++--
 include/linux/dma-iommu.h  |  6 ++--
 include/linux/dma-mapping.h| 34 +++--
 include/linux/swiotlb.h|  9 +++---
 lib/dma-noop.c |  9 +++---
 lib/swiotlb.c  |  9 +++---
 10 files changed, 104 insertions(+), 93 deletions(-)

diff --git a/arch/arm/include/asm/dma-mapping.h 
b/arch/arm/include/asm/dma-mapping.h
index a83570f10124..07202beed663 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -174,7 +174,7 @@ static inline void dma_mark_clean(void *addr, size_t size) 
{ }
  * to be the device-viewed address.
  */
 extern void *arm_dma_alloc(struct device *dev, size_t size, dma_addr_t *handle,
-  gfp_t gfp, struct dma_attrs *attrs);
+  gfp_t gfp, const struct dma_attrs *attrs);
 
 /**
  * arm_dma_free - free memory allocated by arm_dma_alloc
@@ -191,7 +191,7 @@ extern void *arm_dma_alloc(struct device *dev, size_t size, 
dma_addr_t *handle,
  * during and after this call executing are illegal.
  */
 extern void arm_dma_free(struct device *dev, size_t size, void *cpu_addr,
-dma_addr_t handle, struct dma_attrs *attrs);
+dma_addr_t handle, const struct dma_attrs *attrs);
 
 /**
  * arm_dma_mmap - map a coherent DMA allocation into user space
@@ -208,7 +208,7 @@ extern void arm_dma_free(struct device *dev, size_t size, 
void *cpu_addr,
  */
 extern int arm_dma_mmap(struct device *dev, struct vm_area_struct *vma,
void *cpu_addr, dma_addr_t dma_addr, size_t size,
-   struct dma_attrs *attrs);
+   const struct dma_attrs *attrs);
 
 /*
  * This can be called during early boot to increase the size of the atomic
@@ -262,16 +262,16 @@ extern void dmabounce_unregister_dev(struct device *);
  * The scatter list versions of the above methods.
  */
 extern int arm_dma_map_sg(struct device *, struct scatterlist *, int,
-   enum dma_data_direction, struct dma_attrs *attrs);
+   enum dma_data_direction, const struct dma_attrs *attrs);
 extern void arm_dma_unmap_sg(struct device *, struct scatterlist *, int,
-   enum dma_data_direction, struct dma_attrs *attrs);
+   enum dma_data_direction, const struct dma_attrs *attrs);
 extern void arm_dma_sync_sg_for_cpu(struct device *, struct scatterlist *, int,
enum dma_data_direction);
 extern void arm_dma_sync_sg_for_device(struct device *, struct scatterlist *, 
int,
enum dma_data_direction);
 extern int arm_dma_get_sgtable(struct device *dev, struct sg_table *sgt,
void *cpu_addr, dma_addr_t dma_addr, size_t size,
-   struct dma_attrs *attrs);
+   const struct dma_attrs *attrs);
 
 #endif /* __KERNEL__ */
 #endif
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 4abc50952451..245954e7e343 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -124,7 +124,7 @@ static void __dma_page_dev_to_cpu(struct page *, unsigned 
long,
  */
 static dma_addr_t arm_dma_map_page(struct device *dev, struct page *page,
 unsigned long offset, size_t size, enum dma_data_direction dir,
-struct dma_attrs *attrs)
+const struct dma_attrs *attrs)
 {
if (!dma_get_attr(DMA_ATTR_SKIP_CPU_SYNC, attrs))
__dma_page_cpu_to_dev(page, offset, size, dir);
@@ -133,7 +133,7 @@ static dma_addr_t arm_dma_map_page(struct device *dev, 
struct page *page,
 
 static dma_addr_t arm_coherent_dma_map_page(struct device *dev, struct page 
*page,
 unsigned long offset, size_t size, enum dma_data_direction dir,
-struct dma_attrs *attrs)
+const struct dma_attrs *attrs)
 {
return pfn_to_dma(dev, page_to_pfn(page)) + offset;
 }
@@ -154,7 +154,7 @@ static dma_addr_t arm_coherent_dma_map_page(struct device 
*dev, struct page *pag
  */
 static void arm_dma_unmap_page(struct device *dev, dma_addr_t handle,

[PATCH 0/4] dma-mapping: Constify dma_attrs

[Krzysztof Kozlowski]

Hi,

The patchset is divided into two parts:
1. (patch 1-3): Constify dma_attrs passed to some of functions. The
   first patch is a dependency for all other. This is not intrusive.

2. patch 4: request for comments, constify dma_attrs everywhere (struct
   dma_map_ops and implementations).
   Constness may protect from certain coding bugs and, what is more
   important, is a direct documentation of how the core deals
   with passed data. Some of the dma-mapping users allocate attrs
   on the stack so the ownership cannot be transferred. 'Const' here
   means exactly that - the ownership stays with the caller.

   Unfortunately this cannot be done separately per-implementation
   without introducing build warnings so the patch touches multiple
   subsystems. Maybe using some casts in intermediate steps might help
   splitting it into separate patches?

   This is not finished because there is no point of my effort if
   I will hear short NACK soon. :)

Comments are welcomed.

Best regards,
Krzysztof

Krzysztof Kozlowski (4):
  dma-mapping: Constify attrs passed to dma_get_attr
  ARM: dma-mapping: Constify attrs passed to internal functions
  arm64: dma-mapping: Constify attrs passed to internal functions
  dma-mapping: Constify dma_attrs

 arch/arm/include/asm/dma-mapping.h | 12 +++---
 arch/arm/mm/dma-mapping.c  | 87 --
 arch/arm/xen/mm.c  |  4 +-
 arch/arm64/mm/dma-mapping.c| 53 +++
 drivers/iommu/dma-iommu.c  |  6 +--
 include/linux/dma-attrs.h  |  3 +-
 include/linux/dma-iommu.h  |  6 +--
 include/linux/dma-mapping.h| 34 ---
 include/linux/swiotlb.h|  9 ++--
 lib/dma-noop.c |  9 ++--
 lib/swiotlb.c  |  9 ++--
 11 files changed, 123 insertions(+), 109 deletions(-)

-- 
1.9.1

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

[PATCH 2/4] ARM: dma-mapping: Constify attrs passed to internal functions

[Krzysztof Kozlowski]

Some of the non-exported functions do not modify passed dma_attrs so the
pointer can point to const data.

Signed-off-by: Krzysztof Kozlowski 
---
 arch/arm/mm/dma-mapping.c | 26 ++
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index ff7ed5697d3e..4abc50952451 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -621,7 +621,7 @@ static void __free_from_contiguous(struct device *dev, 
struct page *page,
dma_release_from_contiguous(dev, page, size >> PAGE_SHIFT);
 }
 
-static inline pgprot_t __get_dma_pgprot(struct dma_attrs *attrs, pgprot_t prot)
+static inline pgprot_t __get_dma_pgprot(const struct dma_attrs *attrs, 
pgprot_t prot)
 {
prot = dma_get_attr(DMA_ATTR_WRITE_COMBINE, attrs) ?
pgprot_writecombine(prot) :
@@ -732,7 +732,7 @@ static struct arm_dma_allocator remap_allocator = {
 
 static void *__dma_alloc(struct device *dev, size_t size, dma_addr_t *handle,
 gfp_t gfp, pgprot_t prot, bool is_coherent,
-struct dma_attrs *attrs, const void *caller)
+const struct dma_attrs *attrs, const void *caller)
 {
u64 mask = get_coherent_dma_mask(dev);
struct page *page = NULL;
@@ -878,7 +878,7 @@ int arm_dma_mmap(struct device *dev, struct vm_area_struct 
*vma,
  * Free a buffer as defined by the above mapping.
  */
 static void __arm_dma_free(struct device *dev, size_t size, void *cpu_addr,
-  dma_addr_t handle, struct dma_attrs *attrs,
+  dma_addr_t handle, const struct dma_attrs *attrs,
   bool is_coherent)
 {
struct page *page = pfn_to_page(dma_to_pfn(dev, handle));
@@ -1253,7 +1253,8 @@ static inline void __free_iova(struct dma_iommu_mapping 
*mapping,
 static const int iommu_order_array[] = { 9, 8, 4, 0 };
 
 static struct page **__iommu_alloc_buffer(struct device *dev, size_t size,
- gfp_t gfp, struct dma_attrs *attrs)
+ gfp_t gfp,
+ const struct dma_attrs *attrs)
 {
struct page **pages;
int count = size >> PAGE_SHIFT;
@@ -1342,7 +1343,7 @@ error:
 }
 
 static int __iommu_free_buffer(struct device *dev, struct page **pages,
-  size_t size, struct dma_attrs *attrs)
+  size_t size, const struct dma_attrs *attrs)
 {
int count = size >> PAGE_SHIFT;
int i;
@@ -1439,7 +1440,8 @@ static struct page **__atomic_get_pages(void *addr)
return (struct page **)page;
 }
 
-static struct page **__iommu_get_pages(void *cpu_addr, struct dma_attrs *attrs)
+static struct page **__iommu_get_pages(void *cpu_addr,
+  const struct dma_attrs *attrs)
 {
struct vm_struct *area;
 
@@ -1633,8 +1635,8 @@ static int __dma_direction_to_prot(enum 
dma_data_direction dir)
  */
 static int __map_sg_chunk(struct device *dev, struct scatterlist *sg,
  size_t size, dma_addr_t *handle,
- enum dma_data_direction dir, struct dma_attrs *attrs,
- bool is_coherent)
+ enum dma_data_direction dir,
+ const struct dma_attrs *attrs, bool is_coherent)
 {
struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
dma_addr_t iova, iova_base;
@@ -1676,8 +1678,8 @@ fail:
 }
 
 static int __iommu_map_sg(struct device *dev, struct scatterlist *sg, int 
nents,
-enum dma_data_direction dir, struct dma_attrs *attrs,
-bool is_coherent)
+enum dma_data_direction dir,
+const struct dma_attrs *attrs, bool is_coherent)
 {
struct scatterlist *s = sg, *dma = sg, *start = sg;
int i, count = 0;
@@ -1758,8 +1760,8 @@ int arm_iommu_map_sg(struct device *dev, struct 
scatterlist *sg,
 }
 
 static void __iommu_unmap_sg(struct device *dev, struct scatterlist *sg,
-   int nents, enum dma_data_direction dir, struct dma_attrs *attrs,
-   bool is_coherent)
+   int nents, enum dma_data_direction dir,
+   const struct dma_attrs *attrs, bool is_coherent)
 {
struct scatterlist *s;
int i;
-- 
1.9.1

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