Re: [RFC PATCH 09/11] drivers: acpi: implement acpi_dma_configure

[Tomasz Nowicki]

On 17.05.2016 10:07, Tomasz Nowicki wrote:

Hi Lorenzo,

On 14.04.2016 19:25, Lorenzo Pieralisi wrote:

On DT based systems, the of_dma_configure() API implements DMA
configuration
for a given device. On ACPI systems an API equivalent to
of_dma_configure()
is missing which implies that it is currently not possible to set-up DMA
operations for devices through the ACPI generic kernel layer.

This patch fills the gap by introducing acpi_dma_configure/deconfigure()
calls, that carry out IOMMU configuration through IORT (on systems where
it is present) and call arch_setup_dma_ops(...) with the retrieved
parameters.

The DMA range size passed to arch_setup_dma_ops() is sized according
to the device coherent_dma_mask (starting at address 0x0), mirroring the
DT probing path behaviour when a dma-ranges property is not provided
for the device being probed; this changes the current
arch_setup_dma_ops()
call parameters in the ACPI probing case, but since arch_setup_dma_ops()
is a NOP on all architectures but ARM/ARM64 this patch does not change
the current kernel behaviour on them.

This patch updates ACPI and PCI core code to use the newly introduced
acpi_dma_configure function, providing the same functionality
as of_dma_configure on ARM systems and leaving behaviour unchanged
for all other arches.

Signed-off-by: Lorenzo Pieralisi 
Cc: Bjorn Helgaas 
Cc: Robin Murphy 
Cc: Tomasz Nowicki 
Cc: Joerg Roedel 
Cc: "Rafael J. Wysocki" 
---
  drivers/acpi/glue.c |  4 +--
  drivers/acpi/iort.c | 85
+
  drivers/acpi/scan.c | 29 +
  drivers/pci/probe.c |  3 +-
  include/acpi/acpi_bus.h |  2 ++
  include/linux/acpi.h|  5 +++
  include/linux/iort.h|  9 ++
  7 files changed, 133 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
index 5ea5dc2..f8d6564 100644
--- a/drivers/acpi/glue.c
+++ b/drivers/acpi/glue.c
@@ -227,8 +227,7 @@ int acpi_bind_one(struct device *dev, struct
acpi_device *acpi_dev)

  attr = acpi_get_dma_attr(acpi_dev);
  if (attr != DEV_DMA_NOT_SUPPORTED)
-arch_setup_dma_ops(dev, 0, 0, NULL,
-   attr == DEV_DMA_COHERENT);
+acpi_dma_configure(dev, attr);

  acpi_physnode_link_name(physical_node_name, node_id);
  retval = sysfs_create_link(_dev->dev.kobj, >kobj,
@@ -251,6 +250,7 @@ int acpi_bind_one(struct device *dev, struct
acpi_device *acpi_dev)
  return 0;

   err:
+acpi_dma_deconfigure(dev);
  ACPI_COMPANION_SET(dev, NULL);
  put_device(dev);
  put_device(_dev->dev);
diff --git a/drivers/acpi/iort.c b/drivers/acpi/iort.c
index 2b5ce65..b1bb8fb 100644
--- a/drivers/acpi/iort.c
+++ b/drivers/acpi/iort.c
@@ -72,6 +72,31 @@ int iort_iommu_set_node(struct iommu_ops *ops,
struct acpi_iort_node *node,
  return 0;
  }

+/**
+ * iort_iommu_get_node - Retrieve iort_iommu_node associated with an
IORT node.
+ *
+ * @node: IORT table node to be looked-up
+ *
+ * Returns: iort_iommu_node pointer on success
+ *  NULL on failure
+ */
+static struct iort_iommu_node *iort_iommu_get_node(struct
acpi_iort_node *node)
+{
+struct iort_iommu_node *iommu_node;
+
+spin_lock(_iommu_lock);
+list_for_each_entry(iommu_node, _iommu_list, list) {
+if (iommu_node->node == node)
+goto found;
+}
+
+iommu_node = NULL;
+found:
+spin_unlock(_iommu_lock);
+
+return iommu_node;
+}
+
  typedef acpi_status (*iort_find_node_callback)
  (struct acpi_iort_node *node, void *context);

@@ -405,6 +430,66 @@ iort_pci_get_domain(struct pci_dev *pdev, u32
req_id)
  return domain_handle;
  }

+static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)
+{
+u32 *rid = data;
+
+*rid = alias;
+return 0;
+}
+
+/**
+ * iort_iommu_configure - Set-up IOMMU configuration for a device.
+ *
+ * @dev: device that requires IOMMU set-up
+ *
+ * Returns: iommu_ops pointer on configuration success
+ *  NULL on configuration failure
+ */
+struct iommu_ops *iort_iommu_configure(struct device *dev)
+{
+struct acpi_iort_node *node, *parent;
+struct iommu_ops *ops = NULL;
+struct iommu_fwspec fwspec;
+struct iort_iommu_node *iommu_node;
+u32 rid = 0, devid = 0;
+
+if (dev_is_pci(dev)) {
+struct pci_bus *bus = to_pci_dev(dev)->bus;
+
+pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid,
+   );
+
+node = iort_scan_node(ACPI_IORT_NODE_PCI_ROOT_COMPLEX,
+  iort_find_dev_callback, >dev);
+} else
+node = iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT,
+  iort_find_dev_callback, dev);


I think this will not work for finding host bridge stream ID. We still
need to use ACPI_IORT_NODE_PCI_ROOT_COMPLEX but 'dev' is not PCI device,
right?


After private 

Re: [RFC PATCH 09/11] drivers: acpi: implement acpi_dma_configure

[Tomasz Nowicki]

On 16.05.2016 17:15, Tomasz Nowicki wrote:

On 18.04.2016 12:43, Robin Murphy wrote:

On 15/04/16 19:29, Timur Tabi wrote:

On Thu, Apr 14, 2016 at 12:25 PM, Lorenzo Pieralisi
 wrote:

+void acpi_dma_configure(struct device *dev, enum dev_dma_attr attr)
+{
+   struct iommu_ops *iommu;
+
+   iommu = iort_iommu_configure(dev);
+
+   /*
+* Assume dma valid range starts at 0 and covers the whole
+* coherent_dma_mask.
+*/
+   arch_setup_dma_ops(dev, 0, dev->coherent_dma_mask + 1, iommu,
+  attr == DEV_DMA_COHERENT);
+}


I have a network driver that is impacted by this code, so thank you
for posting this. (See
https://www.mail-archive.com/netdev@vger.kernel.org/msg106249.html).

One one SOC, the driver needs to set the mask to 32 bits.  On another
SOC, it needs to set it to 64 bits.  On device tree, the driver will
use dma-ranges.


That's the wrong way to look at it - the driver isn't _using_
dma-ranges, you're merely relying on the OF code setting the _default_
DMA mask differently based on the property. If your driver is in the
minority of those which actually care about DMA masks, then it should be
calling dma_set_mask_and_coherent() appropriately and not relying on the
default.


I don't see the clear strategy for setting DMA mask as well.

Lets consider DT boot method example:
1. SMMUv2 supports 48bit translation and 1:1 address map
dma-ranges = <0x0 0x0 0x0 0x0 0x0001 0x0>;
and we are adding PCI device:

pci_device_add -> DMA_BIT_MASK(32) by default
   pci_dma_configure
 of_dma_configure -> reads dma-ranges and calculates 48bit DMA mask,
but it picks minimum, we stay with DMA_BIT_MASK(32)

now PCI dev turns out to be e1000e NIC:
e1000_probe
   dma_set_mask_and_coherent -> tries to set DMA_BIT_MASK(64)
 dma_set_mask -> there is no set_dma_mask ops for SMMUv2 so we let


Sorry, there is no .set_dma_mask ops for ARM64 (not SMMUv2)

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

Re: [RFC PATCH 09/11] drivers: acpi: implement acpi_dma_configure

[Tomasz Nowicki]

On 18.04.2016 12:43, Robin Murphy wrote:

On 15/04/16 19:29, Timur Tabi wrote:

On Thu, Apr 14, 2016 at 12:25 PM, Lorenzo Pieralisi
 wrote:

+void acpi_dma_configure(struct device *dev, enum dev_dma_attr attr)
+{
+   struct iommu_ops *iommu;
+
+   iommu = iort_iommu_configure(dev);
+
+   /*
+* Assume dma valid range starts at 0 and covers the whole
+* coherent_dma_mask.
+*/
+   arch_setup_dma_ops(dev, 0, dev->coherent_dma_mask + 1, iommu,
+  attr == DEV_DMA_COHERENT);
+}


I have a network driver that is impacted by this code, so thank you
for posting this. (See
https://www.mail-archive.com/netdev@vger.kernel.org/msg106249.html).

One one SOC, the driver needs to set the mask to 32 bits.  On another
SOC, it needs to set it to 64 bits.  On device tree, the driver will
use dma-ranges.


That's the wrong way to look at it - the driver isn't _using_
dma-ranges, you're merely relying on the OF code setting the _default_
DMA mask differently based on the property. If your driver is in the
minority of those which actually care about DMA masks, then it should be
calling dma_set_mask_and_coherent() appropriately and not relying on the
default.


I don't see the clear strategy for setting DMA mask as well.

Lets consider DT boot method example:
1. SMMUv2 supports 48bit translation and 1:1 address map
dma-ranges = <0x0 0x0 0x0 0x0 0x0001 0x0>;
and we are adding PCI device:

pci_device_add -> DMA_BIT_MASK(32) by default
  pci_dma_configure
of_dma_configure -> reads dma-ranges and calculates 48bit DMA mask, 
but it picks minimum, we stay with DMA_BIT_MASK(32)


now PCI dev turns out to be e1000e NIC:
e1000_probe
  dma_set_mask_and_coherent -> tries to set DMA_BIT_MASK(64)
dma_set_mask -> there is no set_dma_mask ops for SMMUv2 so we let 
it be DMA_BIT_MASK(64). From that point on, we let to use memory which 
SMMUv2 cannot work with.


Does lack of set_dma_mask is the only missing thing here?

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

Re: [RFC PATCH 09/11] drivers: acpi: implement acpi_dma_configure

[Lorenzo Pieralisi]

Hi Andy,

On Fri, Apr 22, 2016 at 01:45:38AM +0300, Andy Shevchenko wrote:
> On Thu, Apr 14, 2016 at 8:25 PM, Lorenzo Pieralisi
>  wrote:
> > On DT based systems, the of_dma_configure() API implements DMA configuration
> > for a given device. On ACPI systems an API equivalent to of_dma_configure()
> > is missing which implies that it is currently not possible to set-up DMA
> > operations for devices through the ACPI generic kernel layer.
> >
> > This patch fills the gap by introducing acpi_dma_configure/deconfigure()
> > calls, that carry out IOMMU configuration through IORT (on systems where
> > it is present) and call arch_setup_dma_ops(...) with the retrieved
> > parameters.
> >
> > The DMA range size passed to arch_setup_dma_ops() is sized according
> > to the device coherent_dma_mask (starting at address 0x0), mirroring the
> > DT probing path behaviour when a dma-ranges property is not provided
> > for the device being probed; this changes the current arch_setup_dma_ops()
> > call parameters in the ACPI probing case, but since arch_setup_dma_ops()
> > is a NOP on all architectures but ARM/ARM64 this patch does not change
> > the current kernel behaviour on them.
> >
> > This patch updates ACPI and PCI core code to use the newly introduced
> > acpi_dma_configure function, providing the same functionality
> > as of_dma_configure on ARM systems and leaving behaviour unchanged
> > for all other arches.
> >
> 
> Nitpicks below.

Thanks for having a look.

> > --- a/drivers/acpi/iort.c
> > +++ b/drivers/acpi/iort.c
> > @@ -72,6 +72,31 @@ int iort_iommu_set_node(struct iommu_ops *ops, struct 
> > acpi_iort_node *node,
> > return 0;
> >  }
> >
> > +/**
> > + * iort_iommu_get_node - Retrieve iort_iommu_node associated with an IORT 
> > node.
> > + *
> > + * @node: IORT table node to be looked-up
> > + *
> > + * Returns: iort_iommu_node pointer on success
> > + *  NULL on failure
> > + */
> > +static struct iort_iommu_node *iort_iommu_get_node(struct acpi_iort_node 
> > *node)
> > +{
> > +   struct iort_iommu_node *iommu_node;
> > +
> > +   spin_lock(_iommu_lock);
> > +   list_for_each_entry(iommu_node, _iommu_list, list) {
> > +   if (iommu_node->node == node)
> > +   goto found;
> > +   }
> > +
> > +   iommu_node = NULL;
> > +found:
> > +   spin_unlock(_iommu_lock);
> > +
> > +   return iommu_node;
> 
> Ouch, and why not to
> 
> strut iommu_node = NULL;
> 
> lock
> list for each() {
>  if ()
>   break;
> }
> unlock
> 
> return iommu_node;

To make sure iommu_node is NULL if no node is found, but this list handling
function needs updating anyway (both locking and list handling), so I will
rework it and take your suggestion into account, I agree it is not that
readable (or safe to begin with).

> ?
> 
> > +}
> 
> 
> > +/**
> > + * iort_iommu_configure - Set-up IOMMU configuration for a device.
> > + *
> > + * @dev: device that requires IOMMU set-up
> > + *
> > + * Returns: iommu_ops pointer on configuration success
> > + *  NULL on configuration failure
> > + */
> > +struct iommu_ops *iort_iommu_configure(struct device *dev)
> > +{
> > +   struct acpi_iort_node *node, *parent;
> > +   struct iommu_ops *ops = NULL;
> > +   struct iommu_fwspec fwspec;
> > +   struct iort_iommu_node *iommu_node;
> > +   u32 rid = 0, devid = 0;
> > +
> > +   if (dev_is_pci(dev)) {
> > +   struct pci_bus *bus = to_pci_dev(dev)->bus;
> > +
> > +   pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid,
> > +  );
> > +
> > +   node = iort_scan_node(ACPI_IORT_NODE_PCI_ROOT_COMPLEX,
> > + iort_find_dev_callback, >dev);
> 
> > +   } else
> 
> checkpatch.pl ?

checkpatch.pl --strict does not even barf at it. I will add the braces
and go check why checkpatch.pl is quiet :)

> > +   node = iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT,
> > + iort_find_dev_callback, dev);
> > +
> > +   if (!node)
> > +   return NULL;
> > +
> > +   iort_dev_map_rid(node, rid, , ACPI_IORT_NODE_SMMU);
> > +
> > +   parent = iort_find_parent_node(node, ACPI_IORT_NODE_SMMU);
> 
> > +
> 
> Redundant.

Ok.

Thanks,
Lorenzo

> 

> > +   if (!parent)
> > +   return NULL;
> > +
> > +   iommu_node = iort_iommu_get_node(parent);
> > +   ops = iommu_node->ops;
> > +
> > +   fwspec.fwnode = iommu_node->fwnode;
> > +   fwspec.param_count = 1;
> > +   fwspec.param[0] = devid;
> > +
> > +   if (!ops || !ops->fw_xlate || ops->fw_xlate(dev, ))
> > +   return NULL;
> > +
> > +   return ops;
> > +}
> > +
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH 09/11] drivers: acpi: implement acpi_dma_configure

[Robin Murphy]

On 15/04/16 19:29, Timur Tabi wrote:

On Thu, Apr 14, 2016 at 12:25 PM, Lorenzo Pieralisi
 wrote:

+void acpi_dma_configure(struct device *dev, enum dev_dma_attr attr)
+{
+   struct iommu_ops *iommu;
+
+   iommu = iort_iommu_configure(dev);
+
+   /*
+* Assume dma valid range starts at 0 and covers the whole
+* coherent_dma_mask.
+*/
+   arch_setup_dma_ops(dev, 0, dev->coherent_dma_mask + 1, iommu,
+  attr == DEV_DMA_COHERENT);
+}


I have a network driver that is impacted by this code, so thank you
for posting this. (See
https://www.mail-archive.com/netdev@vger.kernel.org/msg106249.html).

One one SOC, the driver needs to set the mask to 32 bits.  On another
SOC, it needs to set it to 64 bits.  On device tree, the driver will
use dma-ranges.


That's the wrong way to look at it - the driver isn't _using_ 
dma-ranges, you're merely relying on the OF code setting the _default_ 
DMA mask differently based on the property. If your driver is in the 
minority of those which actually care about DMA masks, then it should be 
calling dma_set_mask_and_coherent() appropriately and not relying on the 
default.



In your patches, where is coherent_dma_mask initialized?  I found this
code in add_smmu_platform_device(), but I think this is setting the
mask for the IOMMU driver, not the individual devices.


Yes, that's for the SMMU itself as a device (i.e. the page table walker) 
- as a handy example of "drivers which actually care about DMA masks", 
it specifically needs to avoid a too-small DMA mask because 
bounce-buffering the page tables tends to make things go horribly wrong.


Robin.


 Either way, I
don't understand where the correct value is going to be overridden.

+   /*
+* Set default dma mask value for the table walker,
+* to be overridden on probing with correct value.
+*/
+   *pdev->dev.dma_mask = DMA_BIT_MASK(32);
+   pdev->dev.coherent_dma_mask = *pdev->dev.dma_mask;



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

Re: [RFC PATCH 09/11] drivers: acpi: implement acpi_dma_configure

[Lorenzo Pieralisi]

On Fri, Apr 15, 2016 at 01:29:14PM -0500, Timur Tabi wrote:
> On Thu, Apr 14, 2016 at 12:25 PM, Lorenzo Pieralisi
>  wrote:
> > +void acpi_dma_configure(struct device *dev, enum dev_dma_attr attr)
> > +{
> > +   struct iommu_ops *iommu;
> > +
> > +   iommu = iort_iommu_configure(dev);
> > +
> > +   /*
> > +* Assume dma valid range starts at 0 and covers the whole
> > +* coherent_dma_mask.
> > +*/
> > +   arch_setup_dma_ops(dev, 0, dev->coherent_dma_mask + 1, iommu,
> > +  attr == DEV_DMA_COHERENT);
> > +}
> 
> I have a network driver that is impacted by this code, so thank you
> for posting this. (See
> https://www.mail-archive.com/netdev@vger.kernel.org/msg106249.html).
> 
> One one SOC, the driver needs to set the mask to 32 bits.  On another
> SOC, it needs to set it to 64 bits.  On device tree, the driver will
> use dma-ranges.

First off I think we agree this patch does not change current behaviour
as far as the devices default dma_mask are concerned. They are
initialized in PCI/ACPI core code:

- pci_setup_device()
- acpi_create_platform_device()

As for ACPI DT-dma-ranges equivalent I have to check if I can use
the _DMA method for that so that we can put in place the same
mechanism as DT to override the default masks, other than that it is
up to the drivers to set-up the dma mask accordingly, that's not
something this patchset is changing anyway.

> In your patches, where is coherent_dma_mask initialized?  I found this
> code in add_smmu_platform_device(), but I think this is setting the
> mask for the IOMMU driver, not the individual devices.  Either way, I
> don't understand where the correct value is going to be overridden.

For the ARM SMMU table walker:

arm_smmu_device_cfg_probe() - dma_set_mask_and_coherent()

For other devices see above.

Thanks,
Lorenzo

> 
> +   /*
> +* Set default dma mask value for the table walker,
> +* to be overridden on probing with correct value.
> +*/
> +   *pdev->dev.dma_mask = DMA_BIT_MASK(32);
> +   pdev->dev.coherent_dma_mask = *pdev->dev.dma_mask;
> 
> -- 
> Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project.
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH 09/11] drivers: acpi: implement acpi_dma_configure

[Timur Tabi]

On Thu, Apr 14, 2016 at 12:25 PM, Lorenzo Pieralisi
 wrote:
> +void acpi_dma_configure(struct device *dev, enum dev_dma_attr attr)
> +{
> +   struct iommu_ops *iommu;
> +
> +   iommu = iort_iommu_configure(dev);
> +
> +   /*
> +* Assume dma valid range starts at 0 and covers the whole
> +* coherent_dma_mask.
> +*/
> +   arch_setup_dma_ops(dev, 0, dev->coherent_dma_mask + 1, iommu,
> +  attr == DEV_DMA_COHERENT);
> +}

I have a network driver that is impacted by this code, so thank you
for posting this. (See
https://www.mail-archive.com/netdev@vger.kernel.org/msg106249.html).

One one SOC, the driver needs to set the mask to 32 bits.  On another
SOC, it needs to set it to 64 bits.  On device tree, the driver will
use dma-ranges.

In your patches, where is coherent_dma_mask initialized?  I found this
code in add_smmu_platform_device(), but I think this is setting the
mask for the IOMMU driver, not the individual devices.  Either way, I
don't understand where the correct value is going to be overridden.

+   /*
+* Set default dma mask value for the table walker,
+* to be overridden on probing with correct value.
+*/
+   *pdev->dev.dma_mask = DMA_BIT_MASK(32);
+   pdev->dev.coherent_dma_mask = *pdev->dev.dma_mask;

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH 09/11] drivers: acpi: implement acpi_dma_configure

[Robin Murphy]

Hi Bjorn,

On 15/04/16 17:14, Bjorn Helgaas wrote:

On Thu, Apr 14, 2016 at 06:25:41PM +0100, Lorenzo Pieralisi wrote:

On DT based systems, the of_dma_configure() API implements DMA configuration
for a given device. On ACPI systems an API equivalent to of_dma_configure()
is missing which implies that it is currently not possible to set-up DMA
operations for devices through the ACPI generic kernel layer.

This patch fills the gap by introducing acpi_dma_configure/deconfigure()
calls, that carry out IOMMU configuration through IORT (on systems where
it is present) and call arch_setup_dma_ops(...) with the retrieved
parameters.

The DMA range size passed to arch_setup_dma_ops() is sized according
to the device coherent_dma_mask (starting at address 0x0), mirroring the
DT probing path behaviour when a dma-ranges property is not provided
for the device being probed; this changes the current arch_setup_dma_ops()
call parameters in the ACPI probing case, but since arch_setup_dma_ops()
is a NOP on all architectures but ARM/ARM64 this patch does not change
the current kernel behaviour on them.

This patch updates ACPI and PCI core code to use the newly introduced
acpi_dma_configure function, providing the same functionality
as of_dma_configure on ARM systems and leaving behaviour unchanged
for all other arches.

Signed-off-by: Lorenzo Pieralisi 
Cc: Bjorn Helgaas 
Cc: Robin Murphy 
Cc: Tomasz Nowicki 
Cc: Joerg Roedel 
Cc: "Rafael J. Wysocki" 


There's only a tiny PCI change in this series, so I assume somebody
else will merge all this.  Here's my ack for the PCI part:

Acked-by: Bjorn Helgaas  # for drivers/pci/probe.c 
change

One question on use of pci_for_each_dma_alias() below.


+static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)
+{
+   u32 *rid = data;
+
+   *rid = alias;
+   return 0;
+}
+
+/**
+ * iort_iommu_configure - Set-up IOMMU configuration for a device.
+ *
+ * @dev: device that requires IOMMU set-up
+ *
+ * Returns: iommu_ops pointer on configuration success
+ *  NULL on configuration failure
+ */
+struct iommu_ops *iort_iommu_configure(struct device *dev)
+{
+   struct acpi_iort_node *node, *parent;
+   struct iommu_ops *ops = NULL;
+   struct iommu_fwspec fwspec;
+   struct iort_iommu_node *iommu_node;
+   u32 rid = 0, devid = 0;
+
+   if (dev_is_pci(dev)) {
+   struct pci_bus *bus = to_pci_dev(dev)->bus;
+
+   pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid,
+  );


You end up with only the last DMA alias in "rid".  Is it really true
that you only need to call iort_dev_map_rid() for one of the aliases?


Indeed - all we care about is what things look like by the time they 
come out of the root complex on their way to the the IOMMU, so whatever 
intermediate aliasing _within_ the PCI bus might happen along the way 
doesn't actually matter.


Robin.


+   node = iort_scan_node(ACPI_IORT_NODE_PCI_ROOT_COMPLEX,
+ iort_find_dev_callback, >dev);
+   } else
+   node = iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT,
+ iort_find_dev_callback, dev);
+
+   if (!node)
+   return NULL;
+
+   iort_dev_map_rid(node, rid, , ACPI_IORT_NODE_SMMU);
+
+   parent = iort_find_parent_node(node, ACPI_IORT_NODE_SMMU);
+
+   if (!parent)
+   return NULL;
+
+   iommu_node = iort_iommu_get_node(parent);
+   ops = iommu_node->ops;
+
+   fwspec.fwnode = iommu_node->fwnode;
+   fwspec.param_count = 1;
+   fwspec.param[0] = devid;
+
+   if (!ops || !ops->fw_xlate || ops->fw_xlate(dev, ))
+   return NULL;
+
+   return ops;
+}

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



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

Re: [RFC PATCH 09/11] drivers: acpi: implement acpi_dma_configure

[Bjorn Helgaas]

On Thu, Apr 14, 2016 at 06:25:41PM +0100, Lorenzo Pieralisi wrote:
> On DT based systems, the of_dma_configure() API implements DMA configuration
> for a given device. On ACPI systems an API equivalent to of_dma_configure()
> is missing which implies that it is currently not possible to set-up DMA
> operations for devices through the ACPI generic kernel layer.
> 
> This patch fills the gap by introducing acpi_dma_configure/deconfigure()
> calls, that carry out IOMMU configuration through IORT (on systems where
> it is present) and call arch_setup_dma_ops(...) with the retrieved
> parameters.
> 
> The DMA range size passed to arch_setup_dma_ops() is sized according
> to the device coherent_dma_mask (starting at address 0x0), mirroring the
> DT probing path behaviour when a dma-ranges property is not provided
> for the device being probed; this changes the current arch_setup_dma_ops()
> call parameters in the ACPI probing case, but since arch_setup_dma_ops()
> is a NOP on all architectures but ARM/ARM64 this patch does not change
> the current kernel behaviour on them.
> 
> This patch updates ACPI and PCI core code to use the newly introduced
> acpi_dma_configure function, providing the same functionality
> as of_dma_configure on ARM systems and leaving behaviour unchanged
> for all other arches.
> 
> Signed-off-by: Lorenzo Pieralisi 
> Cc: Bjorn Helgaas 
> Cc: Robin Murphy 
> Cc: Tomasz Nowicki 
> Cc: Joerg Roedel 
> Cc: "Rafael J. Wysocki" 

There's only a tiny PCI change in this series, so I assume somebody
else will merge all this.  Here's my ack for the PCI part:

Acked-by: Bjorn Helgaas    # for drivers/pci/probe.c change

One question on use of pci_for_each_dma_alias() below.

> +static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)
> +{
> + u32 *rid = data;
> +
> + *rid = alias;
> + return 0;
> +}
> +
> +/**
> + * iort_iommu_configure - Set-up IOMMU configuration for a device.
> + *
> + * @dev: device that requires IOMMU set-up
> + *
> + * Returns: iommu_ops pointer on configuration success
> + *  NULL on configuration failure
> + */
> +struct iommu_ops *iort_iommu_configure(struct device *dev)
> +{
> + struct acpi_iort_node *node, *parent;
> + struct iommu_ops *ops = NULL;
> + struct iommu_fwspec fwspec;
> + struct iort_iommu_node *iommu_node;
> + u32 rid = 0, devid = 0;
> +
> + if (dev_is_pci(dev)) {
> + struct pci_bus *bus = to_pci_dev(dev)->bus;
> +
> + pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid,
> +);

You end up with only the last DMA alias in "rid".  Is it really true
that you only need to call iort_dev_map_rid() for one of the aliases?

> + node = iort_scan_node(ACPI_IORT_NODE_PCI_ROOT_COMPLEX,
> +   iort_find_dev_callback, >dev);
> + } else
> + node = iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT,
> +   iort_find_dev_callback, dev);
> +
> + if (!node)
> + return NULL;
> +
> + iort_dev_map_rid(node, rid, , ACPI_IORT_NODE_SMMU);
> +
> + parent = iort_find_parent_node(node, ACPI_IORT_NODE_SMMU);
> +
> + if (!parent)
> + return NULL;
> +
> + iommu_node = iort_iommu_get_node(parent);
> + ops = iommu_node->ops;
> +
> + fwspec.fwnode = iommu_node->fwnode;
> + fwspec.param_count = 1;
> + fwspec.param[0] = devid;
> +
> + if (!ops || !ops->fw_xlate || ops->fw_xlate(dev, ))
> + return NULL;
> +
> + return ops;
> +}
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu