[PATCH 0/3 v2] iommu/fsl: PAMU driver fixes.

[PATCH 0/3 v2] iommu/fsl: PAMU driver fixes.

[Varun Sethi]

The first patch fixes a build failure, when we try to build for a Freescale
platform without PCI support.

The second patch enables a default DMA window for the device, once it's
detached from a domain. In case of vfio, once device is detached from a
guest it can be again used by the host.

The last patch adds the maintainer entry for the Freescale PAMU driver.

Varun Sethi (3):
  iommu/fsl: Factor out PCI specific code.
  iommu/fsl: Enable default DMA window for PCIe devices once detached
  Add maintainers entry for the Freescale PAMU driver.

 MAINTAINERS                     |    7 ++
 drivers/iommu/fsl_pamu.c        |   43 ++++++++++---
 drivers/iommu/fsl_pamu.h        |    1 +
 drivers/iommu/fsl_pamu_domain.c |  134 +++++++++++++++++++++++++--------------
 4 files changed, 128 insertions(+), 57 deletions(-)

-- 
1.7.9.5

[PATCH 1/3 v2] iommu/fsl: Factor out PCI specific code.

[Varun Sethi]

Factor out PCI specific code in the PAMU driver.

Signed-off-by: Varun Sethi <Varun.Sethi@freescale.com>
---
 drivers/iommu/fsl_pamu_domain.c |   88 +++++++++++++++++++--------------------
 1 file changed, 43 insertions(+), 45 deletions(-)

diff --git a/drivers/iommu/fsl_pamu_domain.c b/drivers/iommu/fsl_pamu_domain.c
index c857c30..966ae70 100644
--- a/drivers/iommu/fsl_pamu_domain.c
+++ b/drivers/iommu/fsl_pamu_domain.c
@@ -677,21 +677,15 @@ static int handle_attach_device(struct fsl_dma_domain *dma_domain,
 	return ret;
 }
 
-static int fsl_pamu_attach_device(struct iommu_domain *domain,
-				  struct device *dev)
+static struct device *get_dma_device(struct device *dev)
 {
-	struct fsl_dma_domain *dma_domain = domain->priv;
-	const u32 *liodn;
-	u32 liodn_cnt;
-	int len, ret = 0;
-	struct pci_dev *pdev = NULL;
-	struct pci_controller *pci_ctl;
+	struct device *dma_dev = dev;
+#ifdef CONFIG_PCI
 
-	/*
-	 * Use LIODN of the PCI controller while attaching a
-	 * PCI device.
-	 */
 	if (dev->bus == &pci_bus_type) {
+		struct pci_controller *pci_ctl;
+		struct pci_dev *pdev;
+
 		pdev = to_pci_dev(dev);
 		pci_ctl = pci_bus_to_host(pdev->bus);
 		/*
@@ -699,17 +693,31 @@ static int fsl_pamu_attach_device(struct iommu_domain *domain,
 		 * so we can get the LIODN programmed by
 		 * u-boot.
 		 */
-		dev = pci_ctl->parent;
+		dma_dev = pci_ctl->parent;
 	}
+#endif
+	return dma_dev;
+}
+
+static int fsl_pamu_attach_device(struct iommu_domain *domain,
+				  struct device *dev)
+{
+	struct fsl_dma_domain *dma_domain = domain->priv;
+	struct device *dma_dev;
+	const u32 *liodn;
+	u32 liodn_cnt;
+	int len, ret = 0;
+
+	dma_dev = get_dma_device(dev);
 
-	liodn = of_get_property(dev->of_node, "fsl,liodn", &len);
+	liodn = of_get_property(dma_dev->of_node, "fsl,liodn", &len);
 	if (liodn) {
 		liodn_cnt = len / sizeof(u32);
 		ret = handle_attach_device(dma_domain, dev,
 					 liodn, liodn_cnt);
 	} else {
 		pr_debug("missing fsl,liodn property at %s\n",
-		          dev->of_node->full_name);
+		          dma_dev->of_node->full_name);
 			ret = -EINVAL;
 	}
 
@@ -720,32 +728,18 @@ static void fsl_pamu_detach_device(struct iommu_domain *domain,
 				      struct device *dev)
 {
 	struct fsl_dma_domain *dma_domain = domain->priv;
+	struct device *dma_dev;
 	const u32 *prop;
 	int len;
-	struct pci_dev *pdev = NULL;
-	struct pci_controller *pci_ctl;
 
-	/*
-	 * Use LIODN of the PCI controller while detaching a
-	 * PCI device.
-	 */
-	if (dev->bus == &pci_bus_type) {
-		pdev = to_pci_dev(dev);
-		pci_ctl = pci_bus_to_host(pdev->bus);
-		/*
-		 * make dev point to pci controller device
-		 * so we can get the LIODN programmed by
-		 * u-boot.
-		 */
-		dev = pci_ctl->parent;
-	}
+	dma_dev = get_dma_device(dev);
 
-	prop = of_get_property(dev->of_node, "fsl,liodn", &len);
+	prop = of_get_property(dma_dev->of_node, "fsl,liodn", &len);
 	if (prop)
 		detach_device(dev, dma_domain);
 	else
 		pr_debug("missing fsl,liodn property at %s\n",
-		          dev->of_node->full_name);
+		          dma_dev->of_node->full_name);
 }
 
 static  int configure_domain_geometry(struct iommu_domain *domain, void *data)
@@ -905,6 +899,7 @@ static struct iommu_group *get_device_iommu_group(struct device *dev)
 	return group;
 }
 
+#ifdef CONFIG_PCI
 static  bool check_pci_ctl_endpt_part(struct pci_controller *pci_ctl)
 {
 	u32 version;
@@ -945,13 +940,18 @@ static struct iommu_group *get_shared_pci_device_group(struct pci_dev *pdev)
 	return NULL;
 }
 
-static struct iommu_group *get_pci_device_group(struct pci_dev *pdev)
+static struct iommu_group *get_pci_device_group(struct device *dev)
 {
 	struct pci_controller *pci_ctl;
 	bool pci_endpt_partioning;
 	struct iommu_group *group = NULL;
-	struct pci_dev *bridge, *dma_pdev = NULL;
+	struct pci_dev *bridge, *pdev;
+	struct pci_dev *dma_pdev = NULL;
 
+	pdev = to_pci_dev(dev);
+	/* Don't create device groups for virtual PCI bridges */
+	if (pdev->subordinate)
+		return NULL;
 	pci_ctl = pci_bus_to_host(pdev->bus);
 	pci_endpt_partioning = check_pci_ctl_endpt_part(pci_ctl);
 	/* We can partition PCIe devices so assign device group to the device */
@@ -1044,11 +1044,11 @@ root_bus:
 
 	return group;
 }
+#endif
 
 static int fsl_pamu_add_device(struct device *dev)
 {
 	struct iommu_group *group = NULL;
-	struct pci_dev *pdev;
 	const u32 *prop;
 	int ret, len;
 
@@ -1056,19 +1056,15 @@ static int fsl_pamu_add_device(struct device *dev)
 	 * For platform devices we allocate a separate group for
 	 * each of the devices.
 	 */
-	if (dev->bus == &pci_bus_type) {
-		pdev = to_pci_dev(dev);
-		/* Don't create device groups for virtual PCI bridges */
-		if (pdev->subordinate)
-			return 0;
-
-		group = get_pci_device_group(pdev);
-
-	} else {
+	if (dev->bus == &platform_bus_type) {
 		prop = of_get_property(dev->of_node, "fsl,liodn", &len);
 		if (prop)
 			group = get_device_iommu_group(dev);
 	}
+#ifdef CONFIG_PCI
+	else
+		group = get_pci_device_group(dev);
+#endif
 
 	if (!group || IS_ERR(group))
 		return PTR_ERR(group);
@@ -1166,7 +1162,9 @@ int pamu_domain_init()
 		return ret;
 
 	bus_set_iommu(&platform_bus_type, &fsl_pamu_ops);
+#ifdef CONFIG_PCI
 	bus_set_iommu(&pci_bus_type, &fsl_pamu_ops);
+#endif
 
 	return ret;
 }
-- 
1.7.9.5

[PATCH 2/3 v2] iommu/fsl: Enable default DMA window for PCIe devices

[Varun Sethi]

Once the PCIe device assigned to a guest VM (via VFIO) gets detached from the iommu domain
(when guest terminates), its PAMU table entry is disabled. So, this would prevent the device
from being used once it's assigned back to the host.

This patch allows for creation of a default DMA window corresponding to the device
and subsequently enabling the PAMU table entry. Before we enable the entry, we ensure that
the device's bus master capability is disabled (device quiesced).

Signed-off-by: Varun Sethi <Varun.Sethi@freescale.com>
---
 drivers/iommu/fsl_pamu.c        |   43 ++++++++++++++++++++++++++++--------
 drivers/iommu/fsl_pamu.h        |    1 +
 drivers/iommu/fsl_pamu_domain.c |   46 ++++++++++++++++++++++++++++++++++++---
 3 files changed, 78 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/fsl_pamu.c b/drivers/iommu/fsl_pamu.c
index cba0498..fb4a031 100644
--- a/drivers/iommu/fsl_pamu.c
+++ b/drivers/iommu/fsl_pamu.c
@@ -225,6 +225,21 @@ static struct paace *pamu_get_spaace(struct paace *paace, u32 wnum)
 	return spaace;
 }
 
+/*
+ * Defaul PPAACE settings for an LIODN.
+ */
+static void setup_default_ppaace(struct paace *ppaace)
+{
+	pamu_init_ppaace(ppaace);
+	/* window size is 2^(WSE+1) bytes */
+	set_bf(ppaace->addr_bitfields, PPAACE_AF_WSE, 35);
+	ppaace->wbah = 0;
+	set_bf(ppaace->addr_bitfields, PPAACE_AF_WBAL, 0);
+	set_bf(ppaace->impl_attr, PAACE_IA_ATM,
+		PAACE_ATM_NO_XLATE);
+	set_bf(ppaace->addr_bitfields, PAACE_AF_AP,
+		PAACE_AP_PERMS_ALL);
+}
 /**
  * pamu_get_fspi_and_allocate() - Allocates fspi index and reserves subwindows
  *                                required for primary PAACE in the secondary
@@ -253,6 +268,24 @@ static unsigned long pamu_get_fspi_and_allocate(u32 subwin_cnt)
 	return (spaace_addr - (unsigned long)spaact) / (sizeof(struct paace));
 }
 
+/* Reset the PAACE entry to the default state */
+void enable_default_dma_window(int liodn)
+{
+	struct paace *ppaace;
+
+	ppaace = pamu_get_ppaace(liodn);
+	if (!ppaace) {
+		pr_debug("Invalid liodn entry\n");
+		return;
+	}
+
+	memset(ppaace, 0, sizeof(struct paace));
+
+	setup_default_ppaace(ppaace);
+	mb();
+	pamu_enable_liodn(liodn);
+}
+
 /* Release the subwindows reserved for a particular LIODN */
 void pamu_free_subwins(int liodn)
 {
@@ -752,15 +785,7 @@ static void __init setup_liodns(void)
 				continue;
 			}
 			ppaace = pamu_get_ppaace(liodn);
-			pamu_init_ppaace(ppaace);
-			/* window size is 2^(WSE+1) bytes */
-			set_bf(ppaace->addr_bitfields, PPAACE_AF_WSE, 35);
-			ppaace->wbah = 0;
-			set_bf(ppaace->addr_bitfields, PPAACE_AF_WBAL, 0);
-			set_bf(ppaace->impl_attr, PAACE_IA_ATM,
-				PAACE_ATM_NO_XLATE);
-			set_bf(ppaace->addr_bitfields, PAACE_AF_AP,
-				PAACE_AP_PERMS_ALL);
+			setup_default_ppaace(ppaace);
 			if (of_device_is_compatible(node, "fsl,qman-portal"))
 				setup_qbman_paace(ppaace, QMAN_PORTAL_PAACE);
 			if (of_device_is_compatible(node, "fsl,qman"))
diff --git a/drivers/iommu/fsl_pamu.h b/drivers/iommu/fsl_pamu.h
index 8fc1a12..0edcbbbb 100644
--- a/drivers/iommu/fsl_pamu.h
+++ b/drivers/iommu/fsl_pamu.h
@@ -406,5 +406,6 @@ void get_ome_index(u32 *omi_index, struct device *dev);
 int  pamu_update_paace_stash(int liodn, u32 subwin, u32 value);
 int pamu_disable_spaace(int liodn, u32 subwin);
 u32 pamu_get_max_subwin_cnt(void);
+void enable_default_dma_window(int liodn);
 
 #endif  /* __FSL_PAMU_H */
diff --git a/drivers/iommu/fsl_pamu_domain.c b/drivers/iommu/fsl_pamu_domain.c
index 966ae70..dd6cafc 100644
--- a/drivers/iommu/fsl_pamu_domain.c
+++ b/drivers/iommu/fsl_pamu_domain.c
@@ -340,17 +340,57 @@ static inline struct device_domain_info *find_domain(struct device *dev)
 	return dev->archdata.iommu_domain;
 }
 
+/* Disable device DMA capability and enable default DMA window */
+static void disable_device_dma(struct device_domain_info *info,
+				int enable_dma_window)
+{
+#ifdef CONFIG_PCI
+	if (info->dev->bus == &pci_bus_type) {
+		struct pci_dev *pdev = NULL;
+		pdev = to_pci_dev(info->dev);
+		if (pci_is_enabled(pdev))
+			pci_disable_device(pdev);
+	}
+#endif
+
+	if (enable_dma_window)
+		enable_default_dma_window(info->liodn);
+}
+
+static int check_for_shared_liodn(struct device_domain_info *info)
+{
+	struct device_domain_info *tmp;
+
+	/*
+	 * Sanity check, to ensure that this is not a
+	 * shared LIODN. In case of a PCIe controller
+	 * it's possible that all PCIe devices share
+	 * the same LIODN.
+	 */
+	list_for_each_entry(tmp, &info->domain->devices, link) {
+		if (info->liodn == tmp->liodn)
+			return 1;
+	}
+
+	return 0;
+}
+
 static void remove_device_ref(struct device_domain_info *info, u32 win_cnt)
 {
 	unsigned long flags;
+	int enable_dma_window = 0;
 
 	list_del(&info->link);
 	spin_lock_irqsave(&iommu_lock, flags);
-	if (win_cnt > 1)
-		pamu_free_subwins(info->liodn);
-	pamu_disable_liodn(info->liodn);
+	if (!check_for_shared_liodn(info)) {
+		if (win_cnt > 1)
+			pamu_free_subwins(info->liodn);
+		pamu_disable_liodn(info->liodn);
+		enable_dma_window = 1;
+	}
 	spin_unlock_irqrestore(&iommu_lock, flags);
 	spin_lock_irqsave(&device_domain_lock, flags);
+	disable_device_dma(info, enable_dma_window);
 	info->dev->archdata.iommu_domain = NULL;
 	kmem_cache_free(iommu_devinfo_cache, info);
 	spin_unlock_irqrestore(&device_domain_lock, flags);
-- 
1.7.9.5

[PATCH 3/3] Add maintainers entry for the Freescale PAMU driver.

[Varun Sethi]

Add maintainers entry for Freescale PAMU driver.

Signed-off-by: Varun Sethi <Varun.Sethi@freescale.com>
---
 MAINTAINERS |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 8a0cbf3..5b6ea5c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3511,6 +3511,13 @@ S:	Maintained
 F:	drivers/net/ethernet/freescale/fs_enet/
 F:	include/linux/fs_enet_pd.h
 
+FREESCALE PAMU DRIVER
+M:	Varun Sethi <varun.sethi@freescale.com>
+L:	linuxppc-dev@lists.ozlabs.org
+L:	iommu@lists.linux-foundation.org
+S:	Maintained
+F:	drivers/iommu/fsl_pamu*
+
 FREESCALE QUICC ENGINE LIBRARY
 L:	linuxppc-dev@lists.ozlabs.org
 S:	Orphan
-- 
1.7.9.5

RE: [PATCH 2/3 v2] iommu/fsl: Enable default DMA window for PCIe devices

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Sethi Varun-B16395
> Sent: Wednesday, October 16, 2013 4:53 PM
> To: joro@8bytes.org; iommu@lists.linux-foundation.org; linuxppc-
> dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; Yoder Stuart-B08248; =
Wood
> Scott-B07421; alex.williamson@redhat.com; Bhushan Bharat-R65777
> Cc: Sethi Varun-B16395
> Subject: [PATCH 2/3 v2] iommu/fsl: Enable default DMA window for PCIe dev=
ices
>=20
> Once the PCIe device assigned to a guest VM (via VFIO) gets detached from=
 the
> iommu domain (when guest terminates), its PAMU table entry is disabled. S=
o, this
> would prevent the device from being used once it's assigned back to the h=
ost.
>=20
> This patch allows for creation of a default DMA window corresponding to t=
he
> device and subsequently enabling the PAMU table entry. Before we enable t=
he
> entry, we ensure that the device's bus master capability is disabled (dev=
ice
> quiesced).
>=20
> Signed-off-by: Varun Sethi <Varun.Sethi@freescale.com>
> ---
>  drivers/iommu/fsl_pamu.c        |   43 ++++++++++++++++++++++++++++-----=
---
>  drivers/iommu/fsl_pamu.h        |    1 +
>  drivers/iommu/fsl_pamu_domain.c |   46 +++++++++++++++++++++++++++++++++=
+++---
>  3 files changed, 78 insertions(+), 12 deletions(-)
>=20
> diff --git a/drivers/iommu/fsl_pamu.c b/drivers/iommu/fsl_pamu.c index
> cba0498..fb4a031 100644
> --- a/drivers/iommu/fsl_pamu.c
> +++ b/drivers/iommu/fsl_pamu.c
> @@ -225,6 +225,21 @@ static struct paace *pamu_get_spaace(struct paace *p=
aace,
> u32 wnum)
>  	return spaace;
>  }
>=20
> +/*
> + * Defaul PPAACE settings for an LIODN.
> + */
> +static void setup_default_ppaace(struct paace *ppaace) {
> +	pamu_init_ppaace(ppaace);
> +	/* window size is 2^(WSE+1) bytes */
> +	set_bf(ppaace->addr_bitfields, PPAACE_AF_WSE, 35);
> +	ppaace->wbah =3D 0;
> +	set_bf(ppaace->addr_bitfields, PPAACE_AF_WBAL, 0);
> +	set_bf(ppaace->impl_attr, PAACE_IA_ATM,
> +		PAACE_ATM_NO_XLATE);
> +	set_bf(ppaace->addr_bitfields, PAACE_AF_AP,
> +		PAACE_AP_PERMS_ALL);
> +}
>  /**
>   * pamu_get_fspi_and_allocate() - Allocates fspi index and reserves subw=
indows
>   *                                required for primary PAACE in the seco=
ndary
> @@ -253,6 +268,24 @@ static unsigned long pamu_get_fspi_and_allocate(u32
> subwin_cnt)
>  	return (spaace_addr - (unsigned long)spaact) / (sizeof(struct paace)); =
 }
>=20
> +/* Reset the PAACE entry to the default state */ void
> +enable_default_dma_window(int liodn) {
> +	struct paace *ppaace;
> +
> +	ppaace =3D pamu_get_ppaace(liodn);
> +	if (!ppaace) {
> +		pr_debug("Invalid liodn entry\n");
> +		return;
> +	}
> +
> +	memset(ppaace, 0, sizeof(struct paace));
> +
> +	setup_default_ppaace(ppaace);
> +	mb();
> +	pamu_enable_liodn(liodn);
> +}
> +
>  /* Release the subwindows reserved for a particular LIODN */  void
> pamu_free_subwins(int liodn)  { @@ -752,15 +785,7 @@ static void __init
> setup_liodns(void)
>  				continue;
>  			}
>  			ppaace =3D pamu_get_ppaace(liodn);
> -			pamu_init_ppaace(ppaace);
> -			/* window size is 2^(WSE+1) bytes */
> -			set_bf(ppaace->addr_bitfields, PPAACE_AF_WSE, 35);
> -			ppaace->wbah =3D 0;
> -			set_bf(ppaace->addr_bitfields, PPAACE_AF_WBAL, 0);
> -			set_bf(ppaace->impl_attr, PAACE_IA_ATM,
> -				PAACE_ATM_NO_XLATE);
> -			set_bf(ppaace->addr_bitfields, PAACE_AF_AP,
> -				PAACE_AP_PERMS_ALL);
> +			setup_default_ppaace(ppaace);
>  			if (of_device_is_compatible(node, "fsl,qman-portal"))
>  				setup_qbman_paace(ppaace, QMAN_PORTAL_PAACE);
>  			if (of_device_is_compatible(node, "fsl,qman")) diff --git
> a/drivers/iommu/fsl_pamu.h b/drivers/iommu/fsl_pamu.h index 8fc1a12..0edc=
bbbb
> 100644
> --- a/drivers/iommu/fsl_pamu.h
> +++ b/drivers/iommu/fsl_pamu.h
> @@ -406,5 +406,6 @@ void get_ome_index(u32 *omi_index, struct device *dev=
);  int
> pamu_update_paace_stash(int liodn, u32 subwin, u32 value);  int
> pamu_disable_spaace(int liodn, u32 subwin);
>  u32 pamu_get_max_subwin_cnt(void);
> +void enable_default_dma_window(int liodn);
>=20
>  #endif  /* __FSL_PAMU_H */
> diff --git a/drivers/iommu/fsl_pamu_domain.c b/drivers/iommu/fsl_pamu_dom=
ain.c
> index 966ae70..dd6cafc 100644
> --- a/drivers/iommu/fsl_pamu_domain.c
> +++ b/drivers/iommu/fsl_pamu_domain.c
> @@ -340,17 +340,57 @@ static inline struct device_domain_info
> *find_domain(struct device *dev)
>  	return dev->archdata.iommu_domain;
>  }
>=20
> +/* Disable device DMA capability and enable default DMA window */
> +static void disable_device_dma(struct device_domain_info *info,
> +				int enable_dma_window)
> +{
> +#ifdef CONFIG_PCI
> +	if (info->dev->bus =3D=3D &pci_bus_type) {
> +		struct pci_dev *pdev =3D NULL;
> +		pdev =3D to_pci_dev(info->dev);
> +		if (pci_is_enabled(pdev))
> +			pci_disable_device(pdev);
> +	}
> +#endif
> +
> +	if (enable_dma_window)
> +		enable_default_dma_window(info->liodn);
> +}
> +
> +static int check_for_shared_liodn(struct device_domain_info *info) {
> +	struct device_domain_info *tmp;
> +
> +	/*
> +	 * Sanity check, to ensure that this is not a
> +	 * shared LIODN. In case of a PCIe controller
> +	 * it's possible that all PCIe devices share
> +	 * the same LIODN.
> +	 */
> +	list_for_each_entry(tmp, &info->domain->devices, link) {
> +		if (info->liodn =3D=3D tmp->liodn)
> +			return 1;
> +	}
> +
> +	return 0;
> +}
> +
>  static void remove_device_ref(struct device_domain_info *info, u32 win_c=
nt)  {
>  	unsigned long flags;
> +	int enable_dma_window =3D 0;
>=20
>  	list_del(&info->link);
>  	spin_lock_irqsave(&iommu_lock, flags);
> -	if (win_cnt > 1)
> -		pamu_free_subwins(info->liodn);
> -	pamu_disable_liodn(info->liodn);
> +	if (!check_for_shared_liodn(info)) {

One query; Do we really need to check for this?

Otherwise this patch series looks good to me.

Thanks
-Bharat

> +		if (win_cnt > 1)
> +			pamu_free_subwins(info->liodn);
> +		pamu_disable_liodn(info->liodn);
> +		enable_dma_window =3D 1;
> +	}
>  	spin_unlock_irqrestore(&iommu_lock, flags);
>  	spin_lock_irqsave(&device_domain_lock, flags);
> +	disable_device_dma(info, enable_dma_window);
>  	info->dev->archdata.iommu_domain =3D NULL;
>  	kmem_cache_free(iommu_devinfo_cache, info);
>  	spin_unlock_irqrestore(&device_domain_lock, flags);
> --
> 1.7.9.5

RE: [PATCH 2/3 v2] iommu/fsl: Enable default DMA window for PCIe devices

[Sethi Varun-B16395]

> -----Original Message-----
> From: Bhushan Bharat-R65777
> Sent: Wednesday, October 16, 2013 10:20 PM
> To: Sethi Varun-B16395; joro@8bytes.org; iommu@lists.linux-
> foundation.org; linuxppc-dev@lists.ozlabs.org; linux-
> kernel@vger.kernel.org; Yoder Stuart-B08248; Wood Scott-B07421;
> alex.williamson@redhat.com
> Subject: RE: [PATCH 2/3 v2] iommu/fsl: Enable default DMA window for PCIe
> devices
>=20
>=20
>=20
> > -----Original Message-----
> > From: Sethi Varun-B16395
> > Sent: Wednesday, October 16, 2013 4:53 PM
> > To: joro@8bytes.org; iommu@lists.linux-foundation.org; linuxppc-
> > dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; Yoder
> > Stuart-B08248; Wood Scott-B07421; alex.williamson@redhat.com; Bhushan
> > Bharat-R65777
> > Cc: Sethi Varun-B16395
> > Subject: [PATCH 2/3 v2] iommu/fsl: Enable default DMA window for PCIe
> > devices
> >
> > Once the PCIe device assigned to a guest VM (via VFIO) gets detached
> > from the iommu domain (when guest terminates), its PAMU table entry is
> > disabled. So, this would prevent the device from being used once it's
> assigned back to the host.
> >
> > This patch allows for creation of a default DMA window corresponding
> > to the device and subsequently enabling the PAMU table entry. Before
> > we enable the entry, we ensure that the device's bus master capability
> > is disabled (device quiesced).
> >
> > Signed-off-by: Varun Sethi <Varun.Sethi@freescale.com>
> > ---
> >  drivers/iommu/fsl_pamu.c        |   43 ++++++++++++++++++++++++++++---
> -----
> >  drivers/iommu/fsl_pamu.h        |    1 +
> >  drivers/iommu/fsl_pamu_domain.c |   46
> ++++++++++++++++++++++++++++++++++++---
> >  3 files changed, 78 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/iommu/fsl_pamu.c b/drivers/iommu/fsl_pamu.c index
> > cba0498..fb4a031 100644
> > --- a/drivers/iommu/fsl_pamu.c
> > +++ b/drivers/iommu/fsl_pamu.c
> > @@ -225,6 +225,21 @@ static struct paace *pamu_get_spaace(struct paace
> > *paace,
> > u32 wnum)
> >  	return spaace;
> >  }
> >
> > +/*
> > + * Defaul PPAACE settings for an LIODN.
> > + */
> > +static void setup_default_ppaace(struct paace *ppaace) {
> > +	pamu_init_ppaace(ppaace);
> > +	/* window size is 2^(WSE+1) bytes */
> > +	set_bf(ppaace->addr_bitfields, PPAACE_AF_WSE, 35);
> > +	ppaace->wbah =3D 0;
> > +	set_bf(ppaace->addr_bitfields, PPAACE_AF_WBAL, 0);
> > +	set_bf(ppaace->impl_attr, PAACE_IA_ATM,
> > +		PAACE_ATM_NO_XLATE);
> > +	set_bf(ppaace->addr_bitfields, PAACE_AF_AP,
> > +		PAACE_AP_PERMS_ALL);
> > +}
> >  /**
> >   * pamu_get_fspi_and_allocate() - Allocates fspi index and reserves
> subwindows
> >   *                                required for primary PAACE in the
> secondary
> > @@ -253,6 +268,24 @@ static unsigned long
> > pamu_get_fspi_and_allocate(u32
> > subwin_cnt)
> >  	return (spaace_addr - (unsigned long)spaact) / (sizeof(struct
> > paace));  }
> >
> > +/* Reset the PAACE entry to the default state */ void
> > +enable_default_dma_window(int liodn) {
> > +	struct paace *ppaace;
> > +
> > +	ppaace =3D pamu_get_ppaace(liodn);
> > +	if (!ppaace) {
> > +		pr_debug("Invalid liodn entry\n");
> > +		return;
> > +	}
> > +
> > +	memset(ppaace, 0, sizeof(struct paace));
> > +
> > +	setup_default_ppaace(ppaace);
> > +	mb();
> > +	pamu_enable_liodn(liodn);
> > +}
> > +
> >  /* Release the subwindows reserved for a particular LIODN */  void
> > pamu_free_subwins(int liodn)  { @@ -752,15 +785,7 @@ static void
> > __init
> > setup_liodns(void)
> >  				continue;
> >  			}
> >  			ppaace =3D pamu_get_ppaace(liodn);
> > -			pamu_init_ppaace(ppaace);
> > -			/* window size is 2^(WSE+1) bytes */
> > -			set_bf(ppaace->addr_bitfields, PPAACE_AF_WSE, 35);
> > -			ppaace->wbah =3D 0;
> > -			set_bf(ppaace->addr_bitfields, PPAACE_AF_WBAL, 0);
> > -			set_bf(ppaace->impl_attr, PAACE_IA_ATM,
> > -				PAACE_ATM_NO_XLATE);
> > -			set_bf(ppaace->addr_bitfields, PAACE_AF_AP,
> > -				PAACE_AP_PERMS_ALL);
> > +			setup_default_ppaace(ppaace);
> >  			if (of_device_is_compatible(node, "fsl,qman-portal"))
> >  				setup_qbman_paace(ppaace, QMAN_PORTAL_PAACE);
> >  			if (of_device_is_compatible(node, "fsl,qman")) diff --
> git
> > a/drivers/iommu/fsl_pamu.h b/drivers/iommu/fsl_pamu.h index
> > 8fc1a12..0edcbbbb
> > 100644
> > --- a/drivers/iommu/fsl_pamu.h
> > +++ b/drivers/iommu/fsl_pamu.h
> > @@ -406,5 +406,6 @@ void get_ome_index(u32 *omi_index, struct device
> > *dev);  int pamu_update_paace_stash(int liodn, u32 subwin, u32 value);
> > int pamu_disable_spaace(int liodn, u32 subwin);
> >  u32 pamu_get_max_subwin_cnt(void);
> > +void enable_default_dma_window(int liodn);
> >
> >  #endif  /* __FSL_PAMU_H */
> > diff --git a/drivers/iommu/fsl_pamu_domain.c
> > b/drivers/iommu/fsl_pamu_domain.c index 966ae70..dd6cafc 100644
> > --- a/drivers/iommu/fsl_pamu_domain.c
> > +++ b/drivers/iommu/fsl_pamu_domain.c
> > @@ -340,17 +340,57 @@ static inline struct device_domain_info
> > *find_domain(struct device *dev)
> >  	return dev->archdata.iommu_domain;
> >  }
> >
> > +/* Disable device DMA capability and enable default DMA window */
> > +static void disable_device_dma(struct device_domain_info *info,
> > +				int enable_dma_window)
> > +{
> > +#ifdef CONFIG_PCI
> > +	if (info->dev->bus =3D=3D &pci_bus_type) {
> > +		struct pci_dev *pdev =3D NULL;
> > +		pdev =3D to_pci_dev(info->dev);
> > +		if (pci_is_enabled(pdev))
> > +			pci_disable_device(pdev);
> > +	}
> > +#endif
> > +
> > +	if (enable_dma_window)
> > +		enable_default_dma_window(info->liodn);
> > +}
> > +
> > +static int check_for_shared_liodn(struct device_domain_info *info) {
> > +	struct device_domain_info *tmp;
> > +
> > +	/*
> > +	 * Sanity check, to ensure that this is not a
> > +	 * shared LIODN. In case of a PCIe controller
> > +	 * it's possible that all PCIe devices share
> > +	 * the same LIODN.
> > +	 */
> > +	list_for_each_entry(tmp, &info->domain->devices, link) {
> > +		if (info->liodn =3D=3D tmp->liodn)
> > +			return 1;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static void remove_device_ref(struct device_domain_info *info, u32
> win_cnt)  {
> >  	unsigned long flags;
> > +	int enable_dma_window =3D 0;
> >
> >  	list_del(&info->link);
> >  	spin_lock_irqsave(&iommu_lock, flags);
> > -	if (win_cnt > 1)
> > -		pamu_free_subwins(info->liodn);
> > -	pamu_disable_liodn(info->liodn);
> > +	if (!check_for_shared_liodn(info)) {
>=20
> One query; Do we really need to check for this?
>=20
[Sethi Varun-B16395] Yes, just a sanity check to ensure that there are no m=
ore devices linked to this LIODN and we can disable it.

-Varun

> Otherwise this patch series looks good to me.
>=20
> Thanks
> -Bharat
>=20
> > +		if (win_cnt > 1)
> > +			pamu_free_subwins(info->liodn);
> > +		pamu_disable_liodn(info->liodn);
> > +		enable_dma_window =3D 1;
> > +	}
> >  	spin_unlock_irqrestore(&iommu_lock, flags);
> >  	spin_lock_irqsave(&device_domain_lock, flags);
> > +	disable_device_dma(info, enable_dma_window);
> >  	info->dev->archdata.iommu_domain =3D NULL;
> >  	kmem_cache_free(iommu_devinfo_cache, info);
> >  	spin_unlock_irqrestore(&device_domain_lock, flags);
> > --
> > 1.7.9.5

RE: [PATCH 2/3 v2] iommu/fsl: Enable default DMA window for PCIe devices

[Bhushan Bharat-R65777]

> >
> >
> > > -----Original Message-----
> > > From: Sethi Varun-B16395
> > > Sent: Wednesday, October 16, 2013 4:53 PM
> > > To: joro@8bytes.org; iommu@lists.linux-foundation.org; linuxppc-
> > > dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; Yoder
> > > Stuart-B08248; Wood Scott-B07421; alex.williamson@redhat.com;
> > > Bhushan
> > > Bharat-R65777
> > > Cc: Sethi Varun-B16395
> > > Subject: [PATCH 2/3 v2] iommu/fsl: Enable default DMA window for
> > > PCIe devices
> > >
> > > Once the PCIe device assigned to a guest VM (via VFIO) gets detached
> > > from the iommu domain (when guest terminates), its PAMU table entry
> > > is disabled. So, this would prevent the device from being used once
> > > it's
> > assigned back to the host.
> > >
> > > This patch allows for creation of a default DMA window corresponding
> > > to the device and subsequently enabling the PAMU table entry. Before
> > > we enable the entry, we ensure that the device's bus master
> > > capability is disabled (device quiesced).
> > >
> > > Signed-off-by: Varun Sethi <Varun.Sethi@freescale.com>
> > > ---
> > >  drivers/iommu/fsl_pamu.c        |   43 ++++++++++++++++++++++++++++-=
--
> > -----
> > >  drivers/iommu/fsl_pamu.h        |    1 +
> > >  drivers/iommu/fsl_pamu_domain.c |   46
> > ++++++++++++++++++++++++++++++++++++---
> > >  3 files changed, 78 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/iommu/fsl_pamu.c b/drivers/iommu/fsl_pamu.c
> > > index
> > > cba0498..fb4a031 100644
> > > --- a/drivers/iommu/fsl_pamu.c
> > > +++ b/drivers/iommu/fsl_pamu.c
> > > @@ -225,6 +225,21 @@ static struct paace *pamu_get_spaace(struct
> > > paace *paace,
> > > u32 wnum)
> > >  	return spaace;
> > >  }
> > >
> > > +/*
> > > + * Defaul PPAACE settings for an LIODN.
> > > + */
> > > +static void setup_default_ppaace(struct paace *ppaace) {
> > > +	pamu_init_ppaace(ppaace);
> > > +	/* window size is 2^(WSE+1) bytes */
> > > +	set_bf(ppaace->addr_bitfields, PPAACE_AF_WSE, 35);
> > > +	ppaace->wbah =3D 0;
> > > +	set_bf(ppaace->addr_bitfields, PPAACE_AF_WBAL, 0);
> > > +	set_bf(ppaace->impl_attr, PAACE_IA_ATM,
> > > +		PAACE_ATM_NO_XLATE);
> > > +	set_bf(ppaace->addr_bitfields, PAACE_AF_AP,
> > > +		PAACE_AP_PERMS_ALL);
> > > +}
> > >  /**
> > >   * pamu_get_fspi_and_allocate() - Allocates fspi index and reserves
> > subwindows
> > >   *                                required for primary PAACE in the
> > secondary
> > > @@ -253,6 +268,24 @@ static unsigned long
> > > pamu_get_fspi_and_allocate(u32
> > > subwin_cnt)
> > >  	return (spaace_addr - (unsigned long)spaact) / (sizeof(struct
> > > paace));  }
> > >
> > > +/* Reset the PAACE entry to the default state */ void
> > > +enable_default_dma_window(int liodn) {
> > > +	struct paace *ppaace;
> > > +
> > > +	ppaace =3D pamu_get_ppaace(liodn);
> > > +	if (!ppaace) {
> > > +		pr_debug("Invalid liodn entry\n");
> > > +		return;
> > > +	}
> > > +
> > > +	memset(ppaace, 0, sizeof(struct paace));
> > > +
> > > +	setup_default_ppaace(ppaace);
> > > +	mb();
> > > +	pamu_enable_liodn(liodn);
> > > +}
> > > +
> > >  /* Release the subwindows reserved for a particular LIODN */  void
> > > pamu_free_subwins(int liodn)  { @@ -752,15 +785,7 @@ static void
> > > __init
> > > setup_liodns(void)
> > >  				continue;
> > >  			}
> > >  			ppaace =3D pamu_get_ppaace(liodn);
> > > -			pamu_init_ppaace(ppaace);
> > > -			/* window size is 2^(WSE+1) bytes */
> > > -			set_bf(ppaace->addr_bitfields, PPAACE_AF_WSE, 35);
> > > -			ppaace->wbah =3D 0;
> > > -			set_bf(ppaace->addr_bitfields, PPAACE_AF_WBAL, 0);
> > > -			set_bf(ppaace->impl_attr, PAACE_IA_ATM,
> > > -				PAACE_ATM_NO_XLATE);
> > > -			set_bf(ppaace->addr_bitfields, PAACE_AF_AP,
> > > -				PAACE_AP_PERMS_ALL);
> > > +			setup_default_ppaace(ppaace);
> > >  			if (of_device_is_compatible(node, "fsl,qman-portal"))
> > >  				setup_qbman_paace(ppaace, QMAN_PORTAL_PAACE);
> > >  			if (of_device_is_compatible(node, "fsl,qman")) diff --
> > git
> > > a/drivers/iommu/fsl_pamu.h b/drivers/iommu/fsl_pamu.h index
> > > 8fc1a12..0edcbbbb
> > > 100644
> > > --- a/drivers/iommu/fsl_pamu.h
> > > +++ b/drivers/iommu/fsl_pamu.h
> > > @@ -406,5 +406,6 @@ void get_ome_index(u32 *omi_index, struct device
> > > *dev);  int pamu_update_paace_stash(int liodn, u32 subwin, u32
> > > value); int pamu_disable_spaace(int liodn, u32 subwin);
> > >  u32 pamu_get_max_subwin_cnt(void);
> > > +void enable_default_dma_window(int liodn);
> > >
> > >  #endif  /* __FSL_PAMU_H */
> > > diff --git a/drivers/iommu/fsl_pamu_domain.c
> > > b/drivers/iommu/fsl_pamu_domain.c index 966ae70..dd6cafc 100644
> > > --- a/drivers/iommu/fsl_pamu_domain.c
> > > +++ b/drivers/iommu/fsl_pamu_domain.c
> > > @@ -340,17 +340,57 @@ static inline struct device_domain_info
> > > *find_domain(struct device *dev)
> > >  	return dev->archdata.iommu_domain;  }
> > >
> > > +/* Disable device DMA capability and enable default DMA window */
> > > +static void disable_device_dma(struct device_domain_info *info,
> > > +				int enable_dma_window)
> > > +{
> > > +#ifdef CONFIG_PCI
> > > +	if (info->dev->bus =3D=3D &pci_bus_type) {
> > > +		struct pci_dev *pdev =3D NULL;
> > > +		pdev =3D to_pci_dev(info->dev);
> > > +		if (pci_is_enabled(pdev))
> > > +			pci_disable_device(pdev);
> > > +	}
> > > +#endif
> > > +
> > > +	if (enable_dma_window)
> > > +		enable_default_dma_window(info->liodn);
> > > +}
> > > +
> > > +static int check_for_shared_liodn(struct device_domain_info *info) {
> > > +	struct device_domain_info *tmp;
> > > +
> > > +	/*
> > > +	 * Sanity check, to ensure that this is not a
> > > +	 * shared LIODN. In case of a PCIe controller
> > > +	 * it's possible that all PCIe devices share
> > > +	 * the same LIODN.
> > > +	 */
> > > +	list_for_each_entry(tmp, &info->domain->devices, link) {
> > > +		if (info->liodn =3D=3D tmp->liodn)
> > > +			return 1;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static void remove_device_ref(struct device_domain_info *info, u32
> > win_cnt)  {
> > >  	unsigned long flags;
> > > +	int enable_dma_window =3D 0;
> > >
> > >  	list_del(&info->link);
> > >  	spin_lock_irqsave(&iommu_lock, flags);
> > > -	if (win_cnt > 1)
> > > -		pamu_free_subwins(info->liodn);
> > > -	pamu_disable_liodn(info->liodn);
> > > +	if (!check_for_shared_liodn(info)) {
> >
> > One query; Do we really need to check for this?
> >
> [Sethi Varun-B16395] Yes, just a sanity check to ensure that there are no=
 more
> devices linked to this LIODN and we can disable it.

Varun, trying to understand this; say there are two device under a PCI cont=
roller which share the LIODN of PCI controller,
So both of the device must be unbound from kernel driver and then bind both=
 to VFIO.

Now when guest terminated then remove_device_ref() will be called for both =
of device but the sanity check will pass for the one which will be called l=
ater, is this right?=20

Thanks
-Bharat


>=20
> -Varun
>=20
> > Otherwise this patch series looks good to me.
> >
> > Thanks
> > -Bharat
> >
> > > +		if (win_cnt > 1)
> > > +			pamu_free_subwins(info->liodn);
> > > +		pamu_disable_liodn(info->liodn);
> > > +		enable_dma_window =3D 1;
> > > +	}
> > >  	spin_unlock_irqrestore(&iommu_lock, flags);
> > >  	spin_lock_irqsave(&device_domain_lock, flags);
> > > +	disable_device_dma(info, enable_dma_window);
> > >  	info->dev->archdata.iommu_domain =3D NULL;
> > >  	kmem_cache_free(iommu_devinfo_cache, info);
> > >  	spin_unlock_irqrestore(&device_domain_lock, flags);
> > > --
> > > 1.7.9.5

RE: [PATCH 2/3 v2] iommu/fsl: Enable default DMA window for PCIe devices

[Sethi Varun-B16395]

> -----Original Message-----
> From: Bhushan Bharat-R65777
> Sent: Wednesday, October 16, 2013 10:43 PM
> To: Sethi Varun-B16395; joro@8bytes.org; iommu@lists.linux-
> foundation.org; linuxppc-dev@lists.ozlabs.org; linux-
> kernel@vger.kernel.org; Yoder Stuart-B08248; Wood Scott-B07421;
> alex.williamson@redhat.com
> Subject: RE: [PATCH 2/3 v2] iommu/fsl: Enable default DMA window for PCIe
> devices
>=20
>=20
>=20
> > >
> > >
> > > > -----Original Message-----
> > > > From: Sethi Varun-B16395
> > > > Sent: Wednesday, October 16, 2013 4:53 PM
> > > > To: joro@8bytes.org; iommu@lists.linux-foundation.org; linuxppc-
> > > > dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; Yoder
> > > > Stuart-B08248; Wood Scott-B07421; alex.williamson@redhat.com;
> > > > Bhushan
> > > > Bharat-R65777
> > > > Cc: Sethi Varun-B16395
> > > > Subject: [PATCH 2/3 v2] iommu/fsl: Enable default DMA window for
> > > > PCIe devices
> > > >
> > > > Once the PCIe device assigned to a guest VM (via VFIO) gets
> > > > detached from the iommu domain (when guest terminates), its PAMU
> > > > table entry is disabled. So, this would prevent the device from
> > > > being used once it's
> > > assigned back to the host.
> > > >
> > > > This patch allows for creation of a default DMA window
> > > > corresponding to the device and subsequently enabling the PAMU
> > > > table entry. Before we enable the entry, we ensure that the
> > > > device's bus master capability is disabled (device quiesced).
> > > >
> > > > Signed-off-by: Varun Sethi <Varun.Sethi@freescale.com>
> > > > ---
> > > >  drivers/iommu/fsl_pamu.c        |   43
> ++++++++++++++++++++++++++++---
> > > -----
> > > >  drivers/iommu/fsl_pamu.h        |    1 +
> > > >  drivers/iommu/fsl_pamu_domain.c |   46
> > > ++++++++++++++++++++++++++++++++++++---
> > > >  3 files changed, 78 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/drivers/iommu/fsl_pamu.c b/drivers/iommu/fsl_pamu.c
> > > > index
> > > > cba0498..fb4a031 100644
> > > > --- a/drivers/iommu/fsl_pamu.c
> > > > +++ b/drivers/iommu/fsl_pamu.c
> > > > @@ -225,6 +225,21 @@ static struct paace *pamu_get_spaace(struct
> > > > paace *paace,
> > > > u32 wnum)
> > > >  	return spaace;
> > > >  }
> > > >
> > > > +/*
> > > > + * Defaul PPAACE settings for an LIODN.
> > > > + */
> > > > +static void setup_default_ppaace(struct paace *ppaace) {
> > > > +	pamu_init_ppaace(ppaace);
> > > > +	/* window size is 2^(WSE+1) bytes */
> > > > +	set_bf(ppaace->addr_bitfields, PPAACE_AF_WSE, 35);
> > > > +	ppaace->wbah =3D 0;
> > > > +	set_bf(ppaace->addr_bitfields, PPAACE_AF_WBAL, 0);
> > > > +	set_bf(ppaace->impl_attr, PAACE_IA_ATM,
> > > > +		PAACE_ATM_NO_XLATE);
> > > > +	set_bf(ppaace->addr_bitfields, PAACE_AF_AP,
> > > > +		PAACE_AP_PERMS_ALL);
> > > > +}
> > > >  /**
> > > >   * pamu_get_fspi_and_allocate() - Allocates fspi index and
> > > > reserves
> > > subwindows
> > > >   *                                required for primary PAACE in
> the
> > > secondary
> > > > @@ -253,6 +268,24 @@ static unsigned long
> > > > pamu_get_fspi_and_allocate(u32
> > > > subwin_cnt)
> > > >  	return (spaace_addr - (unsigned long)spaact) / (sizeof(struct
> > > > paace));  }
> > > >
> > > > +/* Reset the PAACE entry to the default state */ void
> > > > +enable_default_dma_window(int liodn) {
> > > > +	struct paace *ppaace;
> > > > +
> > > > +	ppaace =3D pamu_get_ppaace(liodn);
> > > > +	if (!ppaace) {
> > > > +		pr_debug("Invalid liodn entry\n");
> > > > +		return;
> > > > +	}
> > > > +
> > > > +	memset(ppaace, 0, sizeof(struct paace));
> > > > +
> > > > +	setup_default_ppaace(ppaace);
> > > > +	mb();
> > > > +	pamu_enable_liodn(liodn);
> > > > +}
> > > > +
> > > >  /* Release the subwindows reserved for a particular LIODN */
> > > > void pamu_free_subwins(int liodn)  { @@ -752,15 +785,7 @@ static
> > > > void __init
> > > > setup_liodns(void)
> > > >  				continue;
> > > >  			}
> > > >  			ppaace =3D pamu_get_ppaace(liodn);
> > > > -			pamu_init_ppaace(ppaace);
> > > > -			/* window size is 2^(WSE+1) bytes */
> > > > -			set_bf(ppaace->addr_bitfields, PPAACE_AF_WSE,
> 35);
> > > > -			ppaace->wbah =3D 0;
> > > > -			set_bf(ppaace->addr_bitfields, PPAACE_AF_WBAL,
> 0);
> > > > -			set_bf(ppaace->impl_attr, PAACE_IA_ATM,
> > > > -				PAACE_ATM_NO_XLATE);
> > > > -			set_bf(ppaace->addr_bitfields, PAACE_AF_AP,
> > > > -				PAACE_AP_PERMS_ALL);
> > > > +			setup_default_ppaace(ppaace);
> > > >  			if (of_device_is_compatible(node, "fsl,qman-
> portal"))
> > > >  				setup_qbman_paace(ppaace,
> QMAN_PORTAL_PAACE);
> > > >  			if (of_device_is_compatible(node, "fsl,qman"))
> diff --
> > > git
> > > > a/drivers/iommu/fsl_pamu.h b/drivers/iommu/fsl_pamu.h index
> > > > 8fc1a12..0edcbbbb
> > > > 100644
> > > > --- a/drivers/iommu/fsl_pamu.h
> > > > +++ b/drivers/iommu/fsl_pamu.h
> > > > @@ -406,5 +406,6 @@ void get_ome_index(u32 *omi_index, struct
> > > > device *dev);  int pamu_update_paace_stash(int liodn, u32 subwin,
> > > > u32 value); int pamu_disable_spaace(int liodn, u32 subwin);
> > > >  u32 pamu_get_max_subwin_cnt(void);
> > > > +void enable_default_dma_window(int liodn);
> > > >
> > > >  #endif  /* __FSL_PAMU_H */
> > > > diff --git a/drivers/iommu/fsl_pamu_domain.c
> > > > b/drivers/iommu/fsl_pamu_domain.c index 966ae70..dd6cafc 100644
> > > > --- a/drivers/iommu/fsl_pamu_domain.c
> > > > +++ b/drivers/iommu/fsl_pamu_domain.c
> > > > @@ -340,17 +340,57 @@ static inline struct device_domain_info
> > > > *find_domain(struct device *dev)
> > > >  	return dev->archdata.iommu_domain;  }
> > > >
> > > > +/* Disable device DMA capability and enable default DMA window */
> > > > +static void disable_device_dma(struct device_domain_info *info,
> > > > +				int enable_dma_window)
> > > > +{
> > > > +#ifdef CONFIG_PCI
> > > > +	if (info->dev->bus =3D=3D &pci_bus_type) {
> > > > +		struct pci_dev *pdev =3D NULL;
> > > > +		pdev =3D to_pci_dev(info->dev);
> > > > +		if (pci_is_enabled(pdev))
> > > > +			pci_disable_device(pdev);
> > > > +	}
> > > > +#endif
> > > > +
> > > > +	if (enable_dma_window)
> > > > +		enable_default_dma_window(info->liodn);
> > > > +}
> > > > +
> > > > +static int check_for_shared_liodn(struct device_domain_info *info)
> {
> > > > +	struct device_domain_info *tmp;
> > > > +
> > > > +	/*
> > > > +	 * Sanity check, to ensure that this is not a
> > > > +	 * shared LIODN. In case of a PCIe controller
> > > > +	 * it's possible that all PCIe devices share
> > > > +	 * the same LIODN.
> > > > +	 */
> > > > +	list_for_each_entry(tmp, &info->domain->devices, link) {
> > > > +		if (info->liodn =3D=3D tmp->liodn)
> > > > +			return 1;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  static void remove_device_ref(struct device_domain_info *info,
> > > > u32
> > > win_cnt)  {
> > > >  	unsigned long flags;
> > > > +	int enable_dma_window =3D 0;
> > > >
> > > >  	list_del(&info->link);
> > > >  	spin_lock_irqsave(&iommu_lock, flags);
> > > > -	if (win_cnt > 1)
> > > > -		pamu_free_subwins(info->liodn);
> > > > -	pamu_disable_liodn(info->liodn);
> > > > +	if (!check_for_shared_liodn(info)) {
> > >
> > > One query; Do we really need to check for this?
> > >
> > [Sethi Varun-B16395] Yes, just a sanity check to ensure that there are
> > no more devices linked to this LIODN and we can disable it.
>=20
> Varun, trying to understand this; say there are two device under a PCI
> controller which share the LIODN of PCI controller, So both of the device
> must be unbound from kernel driver and then bind both to VFIO.
>=20
> Now when guest terminated then remove_device_ref() will be called for
> both of device but the sanity check will pass for the one which will be
> called later, is this right?
>=20
Yes, when the first device is detached PAMU LIODN table entry is not disabl=
ed. The LIODN would only be disabled once all devices are detached.

-Varun

RE: [PATCH 2/3 v2] iommu/fsl: Enable default DMA window for PCIe devices

[Bhushan Bharat-R65777]

> >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Sethi Varun-B16395
> > > > > Sent: Wednesday, October 16, 2013 4:53 PM
> > > > > To: joro@8bytes.org; iommu@lists.linux-foundation.org; linuxppc-
> > > > > dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; Yoder
> > > > > Stuart-B08248; Wood Scott-B07421; alex.williamson@redhat.com;
> > > > > Bhushan
> > > > > Bharat-R65777
> > > > > Cc: Sethi Varun-B16395
> > > > > Subject: [PATCH 2/3 v2] iommu/fsl: Enable default DMA window for
> > > > > PCIe devices
> > > > >
> > > > > Once the PCIe device assigned to a guest VM (via VFIO) gets
> > > > > detached from the iommu domain (when guest terminates), its PAMU
> > > > > table entry is disabled. So, this would prevent the device from
> > > > > being used once it's
> > > > assigned back to the host.
> > > > >
> > > > > This patch allows for creation of a default DMA window
> > > > > corresponding to the device and subsequently enabling the PAMU
> > > > > table entry. Before we enable the entry, we ensure that the
> > > > > device's bus master capability is disabled (device quiesced).
> > > > >
> > > > > Signed-off-by: Varun Sethi <Varun.Sethi@freescale.com>
> > > > > ---
> > > > >  drivers/iommu/fsl_pamu.c        |   43
> > ++++++++++++++++++++++++++++---
> > > > -----
> > > > >  drivers/iommu/fsl_pamu.h        |    1 +
> > > > >  drivers/iommu/fsl_pamu_domain.c |   46
> > > > ++++++++++++++++++++++++++++++++++++---
> > > > >  3 files changed, 78 insertions(+), 12 deletions(-)
> > > > >
> > > > > diff --git a/drivers/iommu/fsl_pamu.c b/drivers/iommu/fsl_pamu.c
> > > > > index
> > > > > cba0498..fb4a031 100644
> > > > > --- a/drivers/iommu/fsl_pamu.c
> > > > > +++ b/drivers/iommu/fsl_pamu.c
> > > > > @@ -225,6 +225,21 @@ static struct paace *pamu_get_spaace(struct
> > > > > paace *paace,
> > > > > u32 wnum)
> > > > >  	return spaace;
> > > > >  }
> > > > >
> > > > > +/*
> > > > > + * Defaul PPAACE settings for an LIODN.
> > > > > + */
> > > > > +static void setup_default_ppaace(struct paace *ppaace) {
> > > > > +	pamu_init_ppaace(ppaace);
> > > > > +	/* window size is 2^(WSE+1) bytes */
> > > > > +	set_bf(ppaace->addr_bitfields, PPAACE_AF_WSE, 35);
> > > > > +	ppaace->wbah =3D 0;
> > > > > +	set_bf(ppaace->addr_bitfields, PPAACE_AF_WBAL, 0);
> > > > > +	set_bf(ppaace->impl_attr, PAACE_IA_ATM,
> > > > > +		PAACE_ATM_NO_XLATE);
> > > > > +	set_bf(ppaace->addr_bitfields, PAACE_AF_AP,
> > > > > +		PAACE_AP_PERMS_ALL);
> > > > > +}
> > > > >  /**
> > > > >   * pamu_get_fspi_and_allocate() - Allocates fspi index and
> > > > > reserves
> > > > subwindows
> > > > >   *                                required for primary PAACE in
> > the
> > > > secondary
> > > > > @@ -253,6 +268,24 @@ static unsigned long
> > > > > pamu_get_fspi_and_allocate(u32
> > > > > subwin_cnt)
> > > > >  	return (spaace_addr - (unsigned long)spaact) / (sizeof(struct
> > > > > paace));  }
> > > > >
> > > > > +/* Reset the PAACE entry to the default state */ void
> > > > > +enable_default_dma_window(int liodn) {
> > > > > +	struct paace *ppaace;
> > > > > +
> > > > > +	ppaace =3D pamu_get_ppaace(liodn);
> > > > > +	if (!ppaace) {
> > > > > +		pr_debug("Invalid liodn entry\n");
> > > > > +		return;
> > > > > +	}
> > > > > +
> > > > > +	memset(ppaace, 0, sizeof(struct paace));
> > > > > +
> > > > > +	setup_default_ppaace(ppaace);
> > > > > +	mb();
> > > > > +	pamu_enable_liodn(liodn);
> > > > > +}
> > > > > +
> > > > >  /* Release the subwindows reserved for a particular LIODN */
> > > > > void pamu_free_subwins(int liodn)  { @@ -752,15 +785,7 @@ static
> > > > > void __init
> > > > > setup_liodns(void)
> > > > >  				continue;
> > > > >  			}
> > > > >  			ppaace =3D pamu_get_ppaace(liodn);
> > > > > -			pamu_init_ppaace(ppaace);
> > > > > -			/* window size is 2^(WSE+1) bytes */
> > > > > -			set_bf(ppaace->addr_bitfields, PPAACE_AF_WSE,
> > 35);
> > > > > -			ppaace->wbah =3D 0;
> > > > > -			set_bf(ppaace->addr_bitfields, PPAACE_AF_WBAL,
> > 0);
> > > > > -			set_bf(ppaace->impl_attr, PAACE_IA_ATM,
> > > > > -				PAACE_ATM_NO_XLATE);
> > > > > -			set_bf(ppaace->addr_bitfields, PAACE_AF_AP,
> > > > > -				PAACE_AP_PERMS_ALL);
> > > > > +			setup_default_ppaace(ppaace);
> > > > >  			if (of_device_is_compatible(node, "fsl,qman-
> > portal"))
> > > > >  				setup_qbman_paace(ppaace,
> > QMAN_PORTAL_PAACE);
> > > > >  			if (of_device_is_compatible(node, "fsl,qman"))
> > diff --
> > > > git
> > > > > a/drivers/iommu/fsl_pamu.h b/drivers/iommu/fsl_pamu.h index
> > > > > 8fc1a12..0edcbbbb
> > > > > 100644
> > > > > --- a/drivers/iommu/fsl_pamu.h
> > > > > +++ b/drivers/iommu/fsl_pamu.h
> > > > > @@ -406,5 +406,6 @@ void get_ome_index(u32 *omi_index, struct
> > > > > device *dev);  int pamu_update_paace_stash(int liodn, u32
> > > > > subwin,
> > > > > u32 value); int pamu_disable_spaace(int liodn, u32 subwin);
> > > > >  u32 pamu_get_max_subwin_cnt(void);
> > > > > +void enable_default_dma_window(int liodn);
> > > > >
> > > > >  #endif  /* __FSL_PAMU_H */
> > > > > diff --git a/drivers/iommu/fsl_pamu_domain.c
> > > > > b/drivers/iommu/fsl_pamu_domain.c index 966ae70..dd6cafc 100644
> > > > > --- a/drivers/iommu/fsl_pamu_domain.c
> > > > > +++ b/drivers/iommu/fsl_pamu_domain.c
> > > > > @@ -340,17 +340,57 @@ static inline struct device_domain_info
> > > > > *find_domain(struct device *dev)
> > > > >  	return dev->archdata.iommu_domain;  }
> > > > >
> > > > > +/* Disable device DMA capability and enable default DMA window
> > > > > +*/ static void disable_device_dma(struct device_domain_info *inf=
o,
> > > > > +				int enable_dma_window)
> > > > > +{
> > > > > +#ifdef CONFIG_PCI
> > > > > +	if (info->dev->bus =3D=3D &pci_bus_type) {
> > > > > +		struct pci_dev *pdev =3D NULL;
> > > > > +		pdev =3D to_pci_dev(info->dev);
> > > > > +		if (pci_is_enabled(pdev))
> > > > > +			pci_disable_device(pdev);
> > > > > +	}
> > > > > +#endif
> > > > > +
> > > > > +	if (enable_dma_window)
> > > > > +		enable_default_dma_window(info->liodn);
> > > > > +}
> > > > > +
> > > > > +static int check_for_shared_liodn(struct device_domain_info
> > > > > +*info)
> > {
> > > > > +	struct device_domain_info *tmp;
> > > > > +
> > > > > +	/*
> > > > > +	 * Sanity check, to ensure that this is not a
> > > > > +	 * shared LIODN. In case of a PCIe controller
> > > > > +	 * it's possible that all PCIe devices share
> > > > > +	 * the same LIODN.
> > > > > +	 */
> > > > > +	list_for_each_entry(tmp, &info->domain->devices, link) {
> > > > > +		if (info->liodn =3D=3D tmp->liodn)
> > > > > +			return 1;
> > > > > +	}
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > >  static void remove_device_ref(struct device_domain_info *info,
> > > > > u32
> > > > win_cnt)  {
> > > > >  	unsigned long flags;
> > > > > +	int enable_dma_window =3D 0;
> > > > >
> > > > >  	list_del(&info->link);
> > > > >  	spin_lock_irqsave(&iommu_lock, flags);
> > > > > -	if (win_cnt > 1)
> > > > > -		pamu_free_subwins(info->liodn);
> > > > > -	pamu_disable_liodn(info->liodn);
> > > > > +	if (!check_for_shared_liodn(info)) {
> > > >
> > > > One query; Do we really need to check for this?
> > > >
> > > [Sethi Varun-B16395] Yes, just a sanity check to ensure that there
> > > are no more devices linked to this LIODN and we can disable it.
> >
> > Varun, trying to understand this; say there are two device under a PCI
> > controller which share the LIODN of PCI controller, So both of the
> > device must be unbound from kernel driver and then bind both to VFIO.
> >
> > Now when guest terminated then remove_device_ref() will be called for
> > both of device but the sanity check will pass for the one which will
> > be called later, is this right?
> >
> Yes, when the first device is detached PAMU LIODN table entry is not disa=
bled.
> The LIODN would only be disabled once all devices are detached.

Ok, thanks for clarification

Patch series looks good to me.

>=20
> -Varun

RE: [PATCH 2/3 v2] iommu/fsl: Enable default DMA window for PCIe devices

[Sethi Varun-B16395]

> -----Original Message-----
> From: Bhushan Bharat-R65777
> Sent: Wednesday, October 16, 2013 10:52 PM
> To: Sethi Varun-B16395; joro@8bytes.org; iommu@lists.linux-
> foundation.org; linuxppc-dev@lists.ozlabs.org; linux-
> kernel@vger.kernel.org; Yoder Stuart-B08248; Wood Scott-B07421;
> alex.williamson@redhat.com
> Subject: RE: [PATCH 2/3 v2] iommu/fsl: Enable default DMA window for PCIe
> devices
>=20
>=20
> > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Sethi Varun-B16395
> > > > > > Sent: Wednesday, October 16, 2013 4:53 PM
> > > > > > To: joro@8bytes.org; iommu@lists.linux-foundation.org;
> > > > > > linuxppc- dev@lists.ozlabs.org; linux-kernel@vger.kernel.org;
> > > > > > Yoder Stuart-B08248; Wood Scott-B07421;
> > > > > > alex.williamson@redhat.com; Bhushan
> > > > > > Bharat-R65777
> > > > > > Cc: Sethi Varun-B16395
> > > > > > Subject: [PATCH 2/3 v2] iommu/fsl: Enable default DMA window
> > > > > > for PCIe devices
> > > > > >
> > > > > > Once the PCIe device assigned to a guest VM (via VFIO) gets
> > > > > > detached from the iommu domain (when guest terminates), its
> > > > > > PAMU table entry is disabled. So, this would prevent the
> > > > > > device from being used once it's
> > > > > assigned back to the host.
> > > > > >
> > > > > > This patch allows for creation of a default DMA window
> > > > > > corresponding to the device and subsequently enabling the PAMU
> > > > > > table entry. Before we enable the entry, we ensure that the
> > > > > > device's bus master capability is disabled (device quiesced).
> > > > > >
> > > > > > Signed-off-by: Varun Sethi <Varun.Sethi@freescale.com>
> > > > > > ---
> > > > > >  drivers/iommu/fsl_pamu.c        |   43
> > > ++++++++++++++++++++++++++++---
> > > > > -----
> > > > > >  drivers/iommu/fsl_pamu.h        |    1 +
> > > > > >  drivers/iommu/fsl_pamu_domain.c |   46
> > > > > ++++++++++++++++++++++++++++++++++++---
> > > > > >  3 files changed, 78 insertions(+), 12 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/iommu/fsl_pamu.c
> > > > > > b/drivers/iommu/fsl_pamu.c index
> > > > > > cba0498..fb4a031 100644
> > > > > > --- a/drivers/iommu/fsl_pamu.c
> > > > > > +++ b/drivers/iommu/fsl_pamu.c
> > > > > > @@ -225,6 +225,21 @@ static struct paace
> > > > > > *pamu_get_spaace(struct paace *paace,
> > > > > > u32 wnum)
> > > > > >  	return spaace;
> > > > > >  }
> > > > > >
> > > > > > +/*
> > > > > > + * Defaul PPAACE settings for an LIODN.
> > > > > > + */
> > > > > > +static void setup_default_ppaace(struct paace *ppaace) {
> > > > > > +	pamu_init_ppaace(ppaace);
> > > > > > +	/* window size is 2^(WSE+1) bytes */
> > > > > > +	set_bf(ppaace->addr_bitfields, PPAACE_AF_WSE, 35);
> > > > > > +	ppaace->wbah =3D 0;
> > > > > > +	set_bf(ppaace->addr_bitfields, PPAACE_AF_WBAL, 0);
> > > > > > +	set_bf(ppaace->impl_attr, PAACE_IA_ATM,
> > > > > > +		PAACE_ATM_NO_XLATE);
> > > > > > +	set_bf(ppaace->addr_bitfields, PAACE_AF_AP,
> > > > > > +		PAACE_AP_PERMS_ALL);
> > > > > > +}
> > > > > >  /**
> > > > > >   * pamu_get_fspi_and_allocate() - Allocates fspi index and
> > > > > > reserves
> > > > > subwindows
> > > > > >   *                                required for primary PAACE
> in
> > > the
> > > > > secondary
> > > > > > @@ -253,6 +268,24 @@ static unsigned long
> > > > > > pamu_get_fspi_and_allocate(u32
> > > > > > subwin_cnt)
> > > > > >  	return (spaace_addr - (unsigned long)spaact) /
> > > > > > (sizeof(struct paace));  }
> > > > > >
> > > > > > +/* Reset the PAACE entry to the default state */ void
> > > > > > +enable_default_dma_window(int liodn) {
> > > > > > +	struct paace *ppaace;
> > > > > > +
> > > > > > +	ppaace =3D pamu_get_ppaace(liodn);
> > > > > > +	if (!ppaace) {
> > > > > > +		pr_debug("Invalid liodn entry\n");
> > > > > > +		return;
> > > > > > +	}
> > > > > > +
> > > > > > +	memset(ppaace, 0, sizeof(struct paace));
> > > > > > +
> > > > > > +	setup_default_ppaace(ppaace);
> > > > > > +	mb();
> > > > > > +	pamu_enable_liodn(liodn);
> > > > > > +}
> > > > > > +
> > > > > >  /* Release the subwindows reserved for a particular LIODN */
> > > > > > void pamu_free_subwins(int liodn)  { @@ -752,15 +785,7 @@
> > > > > > static void __init
> > > > > > setup_liodns(void)
> > > > > >  				continue;
> > > > > >  			}
> > > > > >  			ppaace =3D pamu_get_ppaace(liodn);
> > > > > > -			pamu_init_ppaace(ppaace);
> > > > > > -			/* window size is 2^(WSE+1) bytes */
> > > > > > -			set_bf(ppaace->addr_bitfields, PPAACE_AF_WSE,
> > > 35);
> > > > > > -			ppaace->wbah =3D 0;
> > > > > > -			set_bf(ppaace->addr_bitfields, PPAACE_AF_WBAL,
> > > 0);
> > > > > > -			set_bf(ppaace->impl_attr, PAACE_IA_ATM,
> > > > > > -				PAACE_ATM_NO_XLATE);
> > > > > > -			set_bf(ppaace->addr_bitfields, PAACE_AF_AP,
> > > > > > -				PAACE_AP_PERMS_ALL);
> > > > > > +			setup_default_ppaace(ppaace);
> > > > > >  			if (of_device_is_compatible(node, "fsl,qman-
> > > portal"))
> > > > > >  				setup_qbman_paace(ppaace,
> > > QMAN_PORTAL_PAACE);
> > > > > >  			if (of_device_is_compatible(node, "fsl,qman"))
> > > diff --
> > > > > git
> > > > > > a/drivers/iommu/fsl_pamu.h b/drivers/iommu/fsl_pamu.h index
> > > > > > 8fc1a12..0edcbbbb
> > > > > > 100644
> > > > > > --- a/drivers/iommu/fsl_pamu.h
> > > > > > +++ b/drivers/iommu/fsl_pamu.h
> > > > > > @@ -406,5 +406,6 @@ void get_ome_index(u32 *omi_index, struct
> > > > > > device *dev);  int pamu_update_paace_stash(int liodn, u32
> > > > > > subwin,
> > > > > > u32 value); int pamu_disable_spaace(int liodn, u32 subwin);
> > > > > >  u32 pamu_get_max_subwin_cnt(void);
> > > > > > +void enable_default_dma_window(int liodn);
> > > > > >
> > > > > >  #endif  /* __FSL_PAMU_H */
> > > > > > diff --git a/drivers/iommu/fsl_pamu_domain.c
> > > > > > b/drivers/iommu/fsl_pamu_domain.c index 966ae70..dd6cafc
> > > > > > 100644
> > > > > > --- a/drivers/iommu/fsl_pamu_domain.c
> > > > > > +++ b/drivers/iommu/fsl_pamu_domain.c
> > > > > > @@ -340,17 +340,57 @@ static inline struct device_domain_info
> > > > > > *find_domain(struct device *dev)
> > > > > >  	return dev->archdata.iommu_domain;  }
> > > > > >
> > > > > > +/* Disable device DMA capability and enable default DMA
> > > > > > +window */ static void disable_device_dma(struct
> device_domain_info *info,
> > > > > > +				int enable_dma_window)
> > > > > > +{
> > > > > > +#ifdef CONFIG_PCI
> > > > > > +	if (info->dev->bus =3D=3D &pci_bus_type) {
> > > > > > +		struct pci_dev *pdev =3D NULL;
> > > > > > +		pdev =3D to_pci_dev(info->dev);
> > > > > > +		if (pci_is_enabled(pdev))
> > > > > > +			pci_disable_device(pdev);
> > > > > > +	}
> > > > > > +#endif
> > > > > > +
> > > > > > +	if (enable_dma_window)
> > > > > > +		enable_default_dma_window(info->liodn);
> > > > > > +}
> > > > > > +
> > > > > > +static int check_for_shared_liodn(struct device_domain_info
> > > > > > +*info)
> > > {
> > > > > > +	struct device_domain_info *tmp;
> > > > > > +
> > > > > > +	/*
> > > > > > +	 * Sanity check, to ensure that this is not a
> > > > > > +	 * shared LIODN. In case of a PCIe controller
> > > > > > +	 * it's possible that all PCIe devices share
> > > > > > +	 * the same LIODN.
> > > > > > +	 */
> > > > > > +	list_for_each_entry(tmp, &info->domain->devices, link) {
> > > > > > +		if (info->liodn =3D=3D tmp->liodn)
> > > > > > +			return 1;
> > > > > > +	}
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +
> > > > > >  static void remove_device_ref(struct device_domain_info
> > > > > > *info,
> > > > > > u32
> > > > > win_cnt)  {
> > > > > >  	unsigned long flags;
> > > > > > +	int enable_dma_window =3D 0;
> > > > > >
> > > > > >  	list_del(&info->link);
> > > > > >  	spin_lock_irqsave(&iommu_lock, flags);
> > > > > > -	if (win_cnt > 1)
> > > > > > -		pamu_free_subwins(info->liodn);
> > > > > > -	pamu_disable_liodn(info->liodn);
> > > > > > +	if (!check_for_shared_liodn(info)) {
> > > > >
> > > > > One query; Do we really need to check for this?
> > > > >
> > > > [Sethi Varun-B16395] Yes, just a sanity check to ensure that there
> > > > are no more devices linked to this LIODN and we can disable it.
> > >
> > > Varun, trying to understand this; say there are two device under a
> > > PCI controller which share the LIODN of PCI controller, So both of
> > > the device must be unbound from kernel driver and then bind both to
> VFIO.
> > >
> > > Now when guest terminated then remove_device_ref() will be called
> > > for both of device but the sanity check will pass for the one which
> > > will be called later, is this right?
> > >
> > Yes, when the first device is detached PAMU LIODN table entry is not
> disabled.
> > The LIODN would only be disabled once all devices are detached.
>=20
> Ok, thanks for clarification
>=20
> Patch series looks good to me.
Thanks.

-Varun

RE: [PATCH 0/3 v2] iommu/fsl: PAMU driver fixes.

[Sethi Varun-B16395]

Hi Joerg,
Please consider these patches for 3.12.

Regards
Varun

> -----Original Message-----
> From: Sethi Varun-B16395
> Sent: Wednesday, October 16, 2013 4:53 PM
> To: joro@8bytes.org; iommu@lists.linux-foundation.org; linuxppc-
> dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; Yoder Stuart-B08248;
> Wood Scott-B07421; alex.williamson@redhat.com; Bhushan Bharat-R65777
> Cc: Sethi Varun-B16395
> Subject: [PATCH 0/3 v2] iommu/fsl: PAMU driver fixes.
>=20
> The first patch fixes a build failure, when we try to build for a
> Freescale platform without PCI support.
>=20
> The second patch enables a default DMA window for the device, once it's
> detached from a domain. In case of vfio, once device is detached from a
> guest it can be again used by the host.
>=20
> The last patch adds the maintainer entry for the Freescale PAMU driver.
>=20
> Varun Sethi (3):
>   iommu/fsl: Factor out PCI specific code.
>   iommu/fsl: Enable default DMA window for PCIe devices once detached
>   Add maintainers entry for the Freescale PAMU driver.
>=20
>  MAINTAINERS                     |    7 ++
>  drivers/iommu/fsl_pamu.c        |   43 ++++++++++---
>  drivers/iommu/fsl_pamu.h        |    1 +
>  drivers/iommu/fsl_pamu_domain.c |  134 +++++++++++++++++++++++++--------
> ------
>  4 files changed, 128 insertions(+), 57 deletions(-)
>=20
> --
> 1.7.9.5

Re: [PATCH 2/3 v2] iommu/fsl: Enable default DMA window for PCIe devices

[Alex Williamson]

On Wed, 2013-10-16 at 16:53 +0530, Varun Sethi wrote:
> Once the PCIe device assigned to a guest VM (via VFIO) gets detached from the iommu domain
> (when guest terminates), its PAMU table entry is disabled. So, this would prevent the device
> from being used once it's assigned back to the host.
> 
> This patch allows for creation of a default DMA window corresponding to the device
> and subsequently enabling the PAMU table entry. Before we enable the entry, we ensure that
> the device's bus master capability is disabled (device quiesced).
> 
> Signed-off-by: Varun Sethi <Varun.Sethi@freescale.com>
> ---
>  drivers/iommu/fsl_pamu.c        |   43 ++++++++++++++++++++++++++++--------
>  drivers/iommu/fsl_pamu.h        |    1 +
>  drivers/iommu/fsl_pamu_domain.c |   46 ++++++++++++++++++++++++++++++++++++---
>  3 files changed, 78 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iommu/fsl_pamu.c b/drivers/iommu/fsl_pamu.c
> index cba0498..fb4a031 100644
> --- a/drivers/iommu/fsl_pamu.c
> +++ b/drivers/iommu/fsl_pamu.c
> @@ -225,6 +225,21 @@ static struct paace *pamu_get_spaace(struct paace *paace, u32 wnum)
>  	return spaace;
>  }
>  
> +/*
> + * Defaul PPAACE settings for an LIODN.
> + */
> +static void setup_default_ppaace(struct paace *ppaace)
> +{
> +	pamu_init_ppaace(ppaace);
> +	/* window size is 2^(WSE+1) bytes */
> +	set_bf(ppaace->addr_bitfields, PPAACE_AF_WSE, 35);
> +	ppaace->wbah = 0;
> +	set_bf(ppaace->addr_bitfields, PPAACE_AF_WBAL, 0);
> +	set_bf(ppaace->impl_attr, PAACE_IA_ATM,
> +		PAACE_ATM_NO_XLATE);
> +	set_bf(ppaace->addr_bitfields, PAACE_AF_AP,
> +		PAACE_AP_PERMS_ALL);
> +}
>  /**
>   * pamu_get_fspi_and_allocate() - Allocates fspi index and reserves subwindows
>   *                                required for primary PAACE in the secondary
> @@ -253,6 +268,24 @@ static unsigned long pamu_get_fspi_and_allocate(u32 subwin_cnt)
>  	return (spaace_addr - (unsigned long)spaact) / (sizeof(struct paace));
>  }
>  
> +/* Reset the PAACE entry to the default state */
> +void enable_default_dma_window(int liodn)
> +{
> +	struct paace *ppaace;
> +
> +	ppaace = pamu_get_ppaace(liodn);
> +	if (!ppaace) {
> +		pr_debug("Invalid liodn entry\n");
> +		return;
> +	}
> +
> +	memset(ppaace, 0, sizeof(struct paace));
> +
> +	setup_default_ppaace(ppaace);
> +	mb();
> +	pamu_enable_liodn(liodn);
> +}
> +
>  /* Release the subwindows reserved for a particular LIODN */
>  void pamu_free_subwins(int liodn)
>  {
> @@ -752,15 +785,7 @@ static void __init setup_liodns(void)
>  				continue;
>  			}
>  			ppaace = pamu_get_ppaace(liodn);
> -			pamu_init_ppaace(ppaace);
> -			/* window size is 2^(WSE+1) bytes */
> -			set_bf(ppaace->addr_bitfields, PPAACE_AF_WSE, 35);
> -			ppaace->wbah = 0;
> -			set_bf(ppaace->addr_bitfields, PPAACE_AF_WBAL, 0);
> -			set_bf(ppaace->impl_attr, PAACE_IA_ATM,
> -				PAACE_ATM_NO_XLATE);
> -			set_bf(ppaace->addr_bitfields, PAACE_AF_AP,
> -				PAACE_AP_PERMS_ALL);
> +			setup_default_ppaace(ppaace);
>  			if (of_device_is_compatible(node, "fsl,qman-portal"))
>  				setup_qbman_paace(ppaace, QMAN_PORTAL_PAACE);
>  			if (of_device_is_compatible(node, "fsl,qman"))
> diff --git a/drivers/iommu/fsl_pamu.h b/drivers/iommu/fsl_pamu.h
> index 8fc1a12..0edcbbbb 100644
> --- a/drivers/iommu/fsl_pamu.h
> +++ b/drivers/iommu/fsl_pamu.h
> @@ -406,5 +406,6 @@ void get_ome_index(u32 *omi_index, struct device *dev);
>  int  pamu_update_paace_stash(int liodn, u32 subwin, u32 value);
>  int pamu_disable_spaace(int liodn, u32 subwin);
>  u32 pamu_get_max_subwin_cnt(void);
> +void enable_default_dma_window(int liodn);
>  
>  #endif  /* __FSL_PAMU_H */
> diff --git a/drivers/iommu/fsl_pamu_domain.c b/drivers/iommu/fsl_pamu_domain.c
> index 966ae70..dd6cafc 100644
> --- a/drivers/iommu/fsl_pamu_domain.c
> +++ b/drivers/iommu/fsl_pamu_domain.c
> @@ -340,17 +340,57 @@ static inline struct device_domain_info *find_domain(struct device *dev)
>  	return dev->archdata.iommu_domain;
>  }
>  
> +/* Disable device DMA capability and enable default DMA window */
> +static void disable_device_dma(struct device_domain_info *info,
> +				int enable_dma_window)

nit, bool?

> +{
> +#ifdef CONFIG_PCI
> +	if (info->dev->bus == &pci_bus_type) {
> +		struct pci_dev *pdev = NULL;

nit, unnecessary initialization

> +		pdev = to_pci_dev(info->dev);
> +		if (pci_is_enabled(pdev))
> +			pci_disable_device(pdev);

This looks suspicious.  What's the case where you're finding devices
where this is needed?  Logically the driver that called
pci_enable_device() should also call pci_disabled_device() when it's
done.  Maybe there's a case to be made that we can't rely on the driver,
but then maybe there should be a pr_debug/info here to notify that
somebody left the device running.  There might also be the question of
whether you should test the busmaster bit in the command register rather
than trusting these indirect paths if it's really for cleanup.  Thanks,

Alex

> +	}
> +#endif
> +
> +	if (enable_dma_window)
> +		enable_default_dma_window(info->liodn);
> +}
> +
> +static int check_for_shared_liodn(struct device_domain_info *info)
> +{
> +	struct device_domain_info *tmp;
> +
> +	/*
> +	 * Sanity check, to ensure that this is not a
> +	 * shared LIODN. In case of a PCIe controller
> +	 * it's possible that all PCIe devices share
> +	 * the same LIODN.
> +	 */
> +	list_for_each_entry(tmp, &info->domain->devices, link) {
> +		if (info->liodn == tmp->liodn)
> +			return 1;
> +	}
> +
> +	return 0;
> +}
> +
>  static void remove_device_ref(struct device_domain_info *info, u32 win_cnt)
>  {
>  	unsigned long flags;
> +	int enable_dma_window = 0;
>  
>  	list_del(&info->link);
>  	spin_lock_irqsave(&iommu_lock, flags);
> -	if (win_cnt > 1)
> -		pamu_free_subwins(info->liodn);
> -	pamu_disable_liodn(info->liodn);
> +	if (!check_for_shared_liodn(info)) {
> +		if (win_cnt > 1)
> +			pamu_free_subwins(info->liodn);
> +		pamu_disable_liodn(info->liodn);
> +		enable_dma_window = 1;
> +	}
>  	spin_unlock_irqrestore(&iommu_lock, flags);
>  	spin_lock_irqsave(&device_domain_lock, flags);
> +	disable_device_dma(info, enable_dma_window);
>  	info->dev->archdata.iommu_domain = NULL;
>  	kmem_cache_free(iommu_devinfo_cache, info);
>  	spin_unlock_irqrestore(&device_domain_lock, flags);

Re: [PATCH 1/3 v2] iommu/fsl: Factor out PCI specific code.

[Alex Williamson]

On Wed, 2013-10-16 at 16:53 +0530, Varun Sethi wrote:
> Factor out PCI specific code in the PAMU driver.
> 
> Signed-off-by: Varun Sethi <Varun.Sethi@freescale.com>
> ---
>  drivers/iommu/fsl_pamu_domain.c |   88 +++++++++++++++++++--------------------
>  1 file changed, 43 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/iommu/fsl_pamu_domain.c b/drivers/iommu/fsl_pamu_domain.c
> index c857c30..966ae70 100644
> --- a/drivers/iommu/fsl_pamu_domain.c
> +++ b/drivers/iommu/fsl_pamu_domain.c
> @@ -677,21 +677,15 @@ static int handle_attach_device(struct fsl_dma_domain *dma_domain,
>  	return ret;
>  }
>  
> -static int fsl_pamu_attach_device(struct iommu_domain *domain,
> -				  struct device *dev)
> +static struct device *get_dma_device(struct device *dev)
>  {
> -	struct fsl_dma_domain *dma_domain = domain->priv;
> -	const u32 *liodn;
> -	u32 liodn_cnt;
> -	int len, ret = 0;
> -	struct pci_dev *pdev = NULL;
> -	struct pci_controller *pci_ctl;
> +	struct device *dma_dev = dev;
> +#ifdef CONFIG_PCI
>  
> -	/*
> -	 * Use LIODN of the PCI controller while attaching a
> -	 * PCI device.
> -	 */
>  	if (dev->bus == &pci_bus_type) {
> +		struct pci_controller *pci_ctl;
> +		struct pci_dev *pdev;
> +
>  		pdev = to_pci_dev(dev);
>  		pci_ctl = pci_bus_to_host(pdev->bus);
>  		/*
> @@ -699,17 +693,31 @@ static int fsl_pamu_attach_device(struct iommu_domain *domain,
>  		 * so we can get the LIODN programmed by
>  		 * u-boot.
>  		 */
> -		dev = pci_ctl->parent;
> +		dma_dev = pci_ctl->parent;
>  	}
> +#endif
> +	return dma_dev;
> +}
> +
> +static int fsl_pamu_attach_device(struct iommu_domain *domain,
> +				  struct device *dev)
> +{
> +	struct fsl_dma_domain *dma_domain = domain->priv;
> +	struct device *dma_dev;
> +	const u32 *liodn;
> +	u32 liodn_cnt;
> +	int len, ret = 0;
> +
> +	dma_dev = get_dma_device(dev);
>  
> -	liodn = of_get_property(dev->of_node, "fsl,liodn", &len);
> +	liodn = of_get_property(dma_dev->of_node, "fsl,liodn", &len);
>  	if (liodn) {
>  		liodn_cnt = len / sizeof(u32);
>  		ret = handle_attach_device(dma_domain, dev,
>  					 liodn, liodn_cnt);
>  	} else {
>  		pr_debug("missing fsl,liodn property at %s\n",
> -		          dev->of_node->full_name);
> +		          dma_dev->of_node->full_name);
>  			ret = -EINVAL;
>  	}
>  
> @@ -720,32 +728,18 @@ static void fsl_pamu_detach_device(struct iommu_domain *domain,
>  				      struct device *dev)
>  {
>  	struct fsl_dma_domain *dma_domain = domain->priv;
> +	struct device *dma_dev;
>  	const u32 *prop;
>  	int len;
> -	struct pci_dev *pdev = NULL;
> -	struct pci_controller *pci_ctl;
>  
> -	/*
> -	 * Use LIODN of the PCI controller while detaching a
> -	 * PCI device.
> -	 */
> -	if (dev->bus == &pci_bus_type) {
> -		pdev = to_pci_dev(dev);
> -		pci_ctl = pci_bus_to_host(pdev->bus);
> -		/*
> -		 * make dev point to pci controller device
> -		 * so we can get the LIODN programmed by
> -		 * u-boot.
> -		 */
> -		dev = pci_ctl->parent;
> -	}
> +	dma_dev = get_dma_device(dev);
>  
> -	prop = of_get_property(dev->of_node, "fsl,liodn", &len);
> +	prop = of_get_property(dma_dev->of_node, "fsl,liodn", &len);
>  	if (prop)
>  		detach_device(dev, dma_domain);
>  	else
>  		pr_debug("missing fsl,liodn property at %s\n",
> -		          dev->of_node->full_name);
> +		          dma_dev->of_node->full_name);
>  }
>  
>  static  int configure_domain_geometry(struct iommu_domain *domain, void *data)
> @@ -905,6 +899,7 @@ static struct iommu_group *get_device_iommu_group(struct device *dev)
>  	return group;
>  }
>  
> +#ifdef CONFIG_PCI
>  static  bool check_pci_ctl_endpt_part(struct pci_controller *pci_ctl)
>  {
>  	u32 version;
> @@ -945,13 +940,18 @@ static struct iommu_group *get_shared_pci_device_group(struct pci_dev *pdev)
>  	return NULL;
>  }
>  
> -static struct iommu_group *get_pci_device_group(struct pci_dev *pdev)
> +static struct iommu_group *get_pci_device_group(struct device *dev)
>  {
>  	struct pci_controller *pci_ctl;
>  	bool pci_endpt_partioning;
>  	struct iommu_group *group = NULL;
> -	struct pci_dev *bridge, *dma_pdev = NULL;
> +	struct pci_dev *bridge, *pdev;
> +	struct pci_dev *dma_pdev = NULL;
>  
> +	pdev = to_pci_dev(dev);
> +	/* Don't create device groups for virtual PCI bridges */
> +	if (pdev->subordinate)
> +		return NULL;
>  	pci_ctl = pci_bus_to_host(pdev->bus);
>  	pci_endpt_partioning = check_pci_ctl_endpt_part(pci_ctl);
>  	/* We can partition PCIe devices so assign device group to the device */
> @@ -1044,11 +1044,11 @@ root_bus:
>  
>  	return group;
>  }
> +#endif
>  
>  static int fsl_pamu_add_device(struct device *dev)
>  {
>  	struct iommu_group *group = NULL;
> -	struct pci_dev *pdev;
>  	const u32 *prop;
>  	int ret, len;
>  
> @@ -1056,19 +1056,15 @@ static int fsl_pamu_add_device(struct device *dev)
>  	 * For platform devices we allocate a separate group for
>  	 * each of the devices.
>  	 */
> -	if (dev->bus == &pci_bus_type) {
> -		pdev = to_pci_dev(dev);
> -		/* Don't create device groups for virtual PCI bridges */
> -		if (pdev->subordinate)
> -			return 0;
> -
> -		group = get_pci_device_group(pdev);
> -
> -	} else {
> +	if (dev->bus == &platform_bus_type) {
>  		prop = of_get_property(dev->of_node, "fsl,liodn", &len);
>  		if (prop)
>  			group = get_device_iommu_group(dev);
>  	}
> +#ifdef CONFIG_PCI
> +	else
> +		group = get_pci_device_group(dev);
> +#endif
>  
>  	if (!group || IS_ERR(group))
>  		return PTR_ERR(group);

It feels like there's a bug here that group is initialized to NULL and
there are a few ways for it to remain NULL through the above code, then
we get here and return PTR_ERR(NULL), which is indistinguishable from a
successful return below.  Perhaps !group is ok, but if so I'd suggest a
separate return block for it.  Thanks,

Alex

> @@ -1166,7 +1162,9 @@ int pamu_domain_init()
>  		return ret;
>  
>  	bus_set_iommu(&platform_bus_type, &fsl_pamu_ops);
> +#ifdef CONFIG_PCI
>  	bus_set_iommu(&pci_bus_type, &fsl_pamu_ops);
> +#endif
>  
>  	return ret;
>  }

RE: [PATCH 2/3 v2] iommu/fsl: Enable default DMA window for PCIe devices

[Varun Sethi]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogQWxleCBXaWxsaWFtc29u
IFttYWlsdG86YWxleC53aWxsaWFtc29uQHJlZGhhdC5jb21dDQo+IFNlbnQ6IFR1ZXNkYXksIERl
Y2VtYmVyIDAzLCAyMDEzIDM6MDQgQU0NCj4gVG86IFNldGhpIFZhcnVuLUIxNjM5NQ0KPiBDYzog
am9yb0A4Ynl0ZXMub3JnOyBpb21tdUBsaXN0cy5saW51eC1mb3VuZGF0aW9uLm9yZzsgbGludXhw
cGMtDQo+IGRldkBsaXN0cy5vemxhYnMub3JnOyBsaW51eC1rZXJuZWxAdmdlci5rZXJuZWwub3Jn
OyBZb2RlciBTdHVhcnQtQjA4MjQ4Ow0KPiBXb29kIFNjb3R0LUIwNzQyMTsgQmh1c2hhbiBCaGFy
YXQtUjY1Nzc3DQo+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggMi8zIHYyXSBpb21tdS9mc2w6IEVuYWJs
ZSBkZWZhdWx0IERNQSB3aW5kb3cgZm9yIFBDSWUNCj4gZGV2aWNlcw0KPiANCj4gT24gV2VkLCAy
MDEzLTEwLTE2IGF0IDE2OjUzICswNTMwLCBWYXJ1biBTZXRoaSB3cm90ZToNCj4gPiBPbmNlIHRo
ZSBQQ0llIGRldmljZSBhc3NpZ25lZCB0byBhIGd1ZXN0IFZNICh2aWEgVkZJTykgZ2V0cyBkZXRh
Y2hlZA0KPiA+IGZyb20gdGhlIGlvbW11IGRvbWFpbiAod2hlbiBndWVzdCB0ZXJtaW5hdGVzKSwg
aXRzIFBBTVUgdGFibGUgZW50cnkgaXMNCj4gPiBkaXNhYmxlZC4gU28sIHRoaXMgd291bGQgcHJl
dmVudCB0aGUgZGV2aWNlIGZyb20gYmVpbmcgdXNlZCBvbmNlIGl0J3MNCj4gYXNzaWduZWQgYmFj
ayB0byB0aGUgaG9zdC4NCj4gPg0KPiA+IFRoaXMgcGF0Y2ggYWxsb3dzIGZvciBjcmVhdGlvbiBv
ZiBhIGRlZmF1bHQgRE1BIHdpbmRvdyBjb3JyZXNwb25kaW5nDQo+ID4gdG8gdGhlIGRldmljZSBh
bmQgc3Vic2VxdWVudGx5IGVuYWJsaW5nIHRoZSBQQU1VIHRhYmxlIGVudHJ5LiBCZWZvcmUNCj4g
PiB3ZSBlbmFibGUgdGhlIGVudHJ5LCB3ZSBlbnN1cmUgdGhhdCB0aGUgZGV2aWNlJ3MgYnVzIG1h
c3RlciBjYXBhYmlsaXR5DQo+IGlzIGRpc2FibGVkIChkZXZpY2UgcXVpZXNjZWQpLg0KPiA+DQo+
ID4gU2lnbmVkLW9mZi1ieTogVmFydW4gU2V0aGkgPFZhcnVuLlNldGhpQGZyZWVzY2FsZS5jb20+
DQo+ID4gLS0tDQo+ID4gIGRyaXZlcnMvaW9tbXUvZnNsX3BhbXUuYyAgICAgICAgfCAgIDQzICsr
KysrKysrKysrKysrKysrKysrKysrKysrKystLS0NCj4gLS0tLS0NCj4gPiAgZHJpdmVycy9pb21t
dS9mc2xfcGFtdS5oICAgICAgICB8ICAgIDEgKw0KPiA+ICBkcml2ZXJzL2lvbW11L2ZzbF9wYW11
X2RvbWFpbi5jIHwgICA0Ng0KPiArKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKyst
LS0NCj4gPiAgMyBmaWxlcyBjaGFuZ2VkLCA3OCBpbnNlcnRpb25zKCspLCAxMiBkZWxldGlvbnMo
LSkNCj4gPg0KPiA+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2lvbW11L2ZzbF9wYW11LmMgYi9kcml2
ZXJzL2lvbW11L2ZzbF9wYW11LmMgaW5kZXgNCj4gPiBjYmEwNDk4Li5mYjRhMDMxIDEwMDY0NA0K
PiA+IC0tLSBhL2RyaXZlcnMvaW9tbXUvZnNsX3BhbXUuYw0KPiA+ICsrKyBiL2RyaXZlcnMvaW9t
bXUvZnNsX3BhbXUuYw0KPiA+IEBAIC0yMjUsNiArMjI1LDIxIEBAIHN0YXRpYyBzdHJ1Y3QgcGFh
Y2UgKnBhbXVfZ2V0X3NwYWFjZShzdHJ1Y3QgcGFhY2UNCj4gKnBhYWNlLCB1MzIgd251bSkNCj4g
PiAgCXJldHVybiBzcGFhY2U7DQo+ID4gIH0NCj4gPg0KPiA+ICsvKg0KPiA+ICsgKiBEZWZhdWwg
UFBBQUNFIHNldHRpbmdzIGZvciBhbiBMSU9ETi4NCj4gPiArICovDQo+ID4gK3N0YXRpYyB2b2lk
IHNldHVwX2RlZmF1bHRfcHBhYWNlKHN0cnVjdCBwYWFjZSAqcHBhYWNlKSB7DQo+ID4gKwlwYW11
X2luaXRfcHBhYWNlKHBwYWFjZSk7DQo+ID4gKwkvKiB3aW5kb3cgc2l6ZSBpcyAyXihXU0UrMSkg
Ynl0ZXMgKi8NCj4gPiArCXNldF9iZihwcGFhY2UtPmFkZHJfYml0ZmllbGRzLCBQUEFBQ0VfQUZf
V1NFLCAzNSk7DQo+ID4gKwlwcGFhY2UtPndiYWggPSAwOw0KPiA+ICsJc2V0X2JmKHBwYWFjZS0+
YWRkcl9iaXRmaWVsZHMsIFBQQUFDRV9BRl9XQkFMLCAwKTsNCj4gPiArCXNldF9iZihwcGFhY2Ut
PmltcGxfYXR0ciwgUEFBQ0VfSUFfQVRNLA0KPiA+ICsJCVBBQUNFX0FUTV9OT19YTEFURSk7DQo+
ID4gKwlzZXRfYmYocHBhYWNlLT5hZGRyX2JpdGZpZWxkcywgUEFBQ0VfQUZfQVAsDQo+ID4gKwkJ
UEFBQ0VfQVBfUEVSTVNfQUxMKTsNCj4gPiArfQ0KPiA+ICAvKioNCj4gPiAgICogcGFtdV9nZXRf
ZnNwaV9hbmRfYWxsb2NhdGUoKSAtIEFsbG9jYXRlcyBmc3BpIGluZGV4IGFuZCByZXNlcnZlcw0K
PiBzdWJ3aW5kb3dzDQo+ID4gICAqICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICByZXF1
aXJlZCBmb3IgcHJpbWFyeSBQQUFDRSBpbiB0aGUNCj4gc2Vjb25kYXJ5DQo+ID4gQEAgLTI1Myw2
ICsyNjgsMjQgQEAgc3RhdGljIHVuc2lnbmVkIGxvbmcNCj4gcGFtdV9nZXRfZnNwaV9hbmRfYWxs
b2NhdGUodTMyIHN1Yndpbl9jbnQpDQo+ID4gIAlyZXR1cm4gKHNwYWFjZV9hZGRyIC0gKHVuc2ln
bmVkIGxvbmcpc3BhYWN0KSAvIChzaXplb2Yoc3RydWN0DQo+ID4gcGFhY2UpKTsgIH0NCj4gPg0K
PiA+ICsvKiBSZXNldCB0aGUgUEFBQ0UgZW50cnkgdG8gdGhlIGRlZmF1bHQgc3RhdGUgKi8gdm9p
ZA0KPiA+ICtlbmFibGVfZGVmYXVsdF9kbWFfd2luZG93KGludCBsaW9kbikgew0KPiA+ICsJc3Ry
dWN0IHBhYWNlICpwcGFhY2U7DQo+ID4gKw0KPiA+ICsJcHBhYWNlID0gcGFtdV9nZXRfcHBhYWNl
KGxpb2RuKTsNCj4gPiArCWlmICghcHBhYWNlKSB7DQo+ID4gKwkJcHJfZGVidWcoIkludmFsaWQg
bGlvZG4gZW50cnlcbiIpOw0KPiA+ICsJCXJldHVybjsNCj4gPiArCX0NCj4gPiArDQo+ID4gKwlt
ZW1zZXQocHBhYWNlLCAwLCBzaXplb2Yoc3RydWN0IHBhYWNlKSk7DQo+ID4gKw0KPiA+ICsJc2V0
dXBfZGVmYXVsdF9wcGFhY2UocHBhYWNlKTsNCj4gPiArCW1iKCk7DQo+ID4gKwlwYW11X2VuYWJs
ZV9saW9kbihsaW9kbik7DQo+ID4gK30NCj4gPiArDQo+ID4gIC8qIFJlbGVhc2UgdGhlIHN1Yndp
bmRvd3MgcmVzZXJ2ZWQgZm9yIGEgcGFydGljdWxhciBMSU9ETiAqLyAgdm9pZA0KPiA+IHBhbXVf
ZnJlZV9zdWJ3aW5zKGludCBsaW9kbikgIHsgQEAgLTc1MiwxNSArNzg1LDcgQEAgc3RhdGljIHZv
aWQNCj4gPiBfX2luaXQgc2V0dXBfbGlvZG5zKHZvaWQpDQo+ID4gIAkJCQljb250aW51ZTsNCj4g
PiAgCQkJfQ0KPiA+ICAJCQlwcGFhY2UgPSBwYW11X2dldF9wcGFhY2UobGlvZG4pOw0KPiA+IC0J
CQlwYW11X2luaXRfcHBhYWNlKHBwYWFjZSk7DQo+ID4gLQkJCS8qIHdpbmRvdyBzaXplIGlzIDJe
KFdTRSsxKSBieXRlcyAqLw0KPiA+IC0JCQlzZXRfYmYocHBhYWNlLT5hZGRyX2JpdGZpZWxkcywg
UFBBQUNFX0FGX1dTRSwgMzUpOw0KPiA+IC0JCQlwcGFhY2UtPndiYWggPSAwOw0KPiA+IC0JCQlz
ZXRfYmYocHBhYWNlLT5hZGRyX2JpdGZpZWxkcywgUFBBQUNFX0FGX1dCQUwsIDApOw0KPiA+IC0J
CQlzZXRfYmYocHBhYWNlLT5pbXBsX2F0dHIsIFBBQUNFX0lBX0FUTSwNCj4gPiAtCQkJCVBBQUNF
X0FUTV9OT19YTEFURSk7DQo+ID4gLQkJCXNldF9iZihwcGFhY2UtPmFkZHJfYml0ZmllbGRzLCBQ
QUFDRV9BRl9BUCwNCj4gPiAtCQkJCVBBQUNFX0FQX1BFUk1TX0FMTCk7DQo+ID4gKwkJCXNldHVw
X2RlZmF1bHRfcHBhYWNlKHBwYWFjZSk7DQo+ID4gIAkJCWlmIChvZl9kZXZpY2VfaXNfY29tcGF0
aWJsZShub2RlLCAiZnNsLHFtYW4tcG9ydGFsIikpDQo+ID4gIAkJCQlzZXR1cF9xYm1hbl9wYWFj
ZShwcGFhY2UsIFFNQU5fUE9SVEFMX1BBQUNFKTsNCj4gPiAgCQkJaWYgKG9mX2RldmljZV9pc19j
b21wYXRpYmxlKG5vZGUsICJmc2wscW1hbiIpKSBkaWZmIC0tDQo+IGdpdA0KPiA+IGEvZHJpdmVy
cy9pb21tdS9mc2xfcGFtdS5oIGIvZHJpdmVycy9pb21tdS9mc2xfcGFtdS5oIGluZGV4DQo+ID4g
OGZjMWExMi4uMGVkY2JiYmIgMTAwNjQ0DQo+ID4gLS0tIGEvZHJpdmVycy9pb21tdS9mc2xfcGFt
dS5oDQo+ID4gKysrIGIvZHJpdmVycy9pb21tdS9mc2xfcGFtdS5oDQo+ID4gQEAgLTQwNiw1ICs0
MDYsNiBAQCB2b2lkIGdldF9vbWVfaW5kZXgodTMyICpvbWlfaW5kZXgsIHN0cnVjdCBkZXZpY2UN
Cj4gPiAqZGV2KTsgIGludCAgcGFtdV91cGRhdGVfcGFhY2Vfc3Rhc2goaW50IGxpb2RuLCB1MzIg
c3Vid2luLCB1MzINCj4gPiB2YWx1ZSk7ICBpbnQgcGFtdV9kaXNhYmxlX3NwYWFjZShpbnQgbGlv
ZG4sIHUzMiBzdWJ3aW4pOw0KPiA+ICB1MzIgcGFtdV9nZXRfbWF4X3N1Yndpbl9jbnQodm9pZCk7
DQo+ID4gK3ZvaWQgZW5hYmxlX2RlZmF1bHRfZG1hX3dpbmRvdyhpbnQgbGlvZG4pOw0KPiA+DQo+
ID4gICNlbmRpZiAgLyogX19GU0xfUEFNVV9IICovDQo+ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMv
aW9tbXUvZnNsX3BhbXVfZG9tYWluLmMNCj4gPiBiL2RyaXZlcnMvaW9tbXUvZnNsX3BhbXVfZG9t
YWluLmMgaW5kZXggOTY2YWU3MC4uZGQ2Y2FmYyAxMDA2NDQNCj4gPiAtLS0gYS9kcml2ZXJzL2lv
bW11L2ZzbF9wYW11X2RvbWFpbi5jDQo+ID4gKysrIGIvZHJpdmVycy9pb21tdS9mc2xfcGFtdV9k
b21haW4uYw0KPiA+IEBAIC0zNDAsMTcgKzM0MCw1NyBAQCBzdGF0aWMgaW5saW5lIHN0cnVjdCBk
ZXZpY2VfZG9tYWluX2luZm8NCj4gKmZpbmRfZG9tYWluKHN0cnVjdCBkZXZpY2UgKmRldikNCj4g
PiAgCXJldHVybiBkZXYtPmFyY2hkYXRhLmlvbW11X2RvbWFpbjsNCj4gPiAgfQ0KPiA+DQo+ID4g
Ky8qIERpc2FibGUgZGV2aWNlIERNQSBjYXBhYmlsaXR5IGFuZCBlbmFibGUgZGVmYXVsdCBETUEg
d2luZG93ICovDQo+ID4gK3N0YXRpYyB2b2lkIGRpc2FibGVfZGV2aWNlX2RtYShzdHJ1Y3QgZGV2
aWNlX2RvbWFpbl9pbmZvICppbmZvLA0KPiA+ICsJCQkJaW50IGVuYWJsZV9kbWFfd2luZG93KQ0K
PiANCj4gbml0LCBib29sPw0KPiANCj4gPiArew0KPiA+ICsjaWZkZWYgQ09ORklHX1BDSQ0KPiA+
ICsJaWYgKGluZm8tPmRldi0+YnVzID09ICZwY2lfYnVzX3R5cGUpIHsNCj4gPiArCQlzdHJ1Y3Qg
cGNpX2RldiAqcGRldiA9IE5VTEw7DQo+IA0KPiBuaXQsIHVubmVjZXNzYXJ5IGluaXRpYWxpemF0
aW9uDQo+IA0KPiA+ICsJCXBkZXYgPSB0b19wY2lfZGV2KGluZm8tPmRldik7DQo+ID4gKwkJaWYg
KHBjaV9pc19lbmFibGVkKHBkZXYpKQ0KPiA+ICsJCQlwY2lfZGlzYWJsZV9kZXZpY2UocGRldik7
DQo+IA0KPiBUaGlzIGxvb2tzIHN1c3BpY2lvdXMuICBXaGF0J3MgdGhlIGNhc2Ugd2hlcmUgeW91
J3JlIGZpbmRpbmcgZGV2aWNlcw0KPiB3aGVyZSB0aGlzIGlzIG5lZWRlZD8gIExvZ2ljYWxseSB0
aGUgZHJpdmVyIHRoYXQgY2FsbGVkDQo+IHBjaV9lbmFibGVfZGV2aWNlKCkgc2hvdWxkIGFsc28g
Y2FsbCBwY2lfZGlzYWJsZWRfZGV2aWNlKCkgd2hlbiBpdCdzDQo+IGRvbmUuICBNYXliZSB0aGVy
ZSdzIGEgY2FzZSB0byBiZSBtYWRlIHRoYXQgd2UgY2FuJ3QgcmVseSBvbiB0aGUgZHJpdmVyLA0K
PiBidXQgdGhlbiBtYXliZSB0aGVyZSBzaG91bGQgYmUgYSBwcl9kZWJ1Zy9pbmZvIGhlcmUgdG8g
bm90aWZ5IHRoYXQNCj4gc29tZWJvZHkgbGVmdCB0aGUgZGV2aWNlIHJ1bm5pbmcuICBUaGVyZSBt
aWdodCBhbHNvIGJlIHRoZSBxdWVzdGlvbiBvZg0KPiB3aGV0aGVyIHlvdSBzaG91bGQgdGVzdCB0
aGUgYnVzbWFzdGVyIGJpdCBpbiB0aGUgY29tbWFuZCByZWdpc3RlciByYXRoZXINCj4gdGhhbiB0
cnVzdGluZyB0aGVzZSBpbmRpcmVjdCBwYXRocyBpZiBpdCdzIHJlYWxseSBmb3IgY2xlYW51cC4g
IFRoYW5rcywNCj4gDQpPbmNlIHRoZSBkZXZpY2UgaXMgZGV0YWNoZWQgZnJvbSB0aGUgZG9tYWlu
LCB3ZSBlbmFibGUgYSBkZWZhdWx0IERNQSB3aW5kb3cgZm9yIHRoZSBkZXZpY2UuIEJlZm9yZSBl
bmFibGluZyB0aGUgZGVmYXVsdCB3aW5kb3csIEkgd2FudCB0byBlbnN1cmUgdGhhdCBkZXZpY2Ug
RE1BIGlzIGRpc2FibGVkLiBWRklPIHN1YnN5c3RlbSBoYW5kbGVzIHRoaXMgZm9yIHZmaW8tcGNp
LiBCdXQsIGNhbiB3ZSBhc3N1bWUgdGhhdCB0aGlzIHdvdWxkIGFsd2F5cyBiZSB0aGUgY2FzZT8N
Cg0KLVZhcnVuDQoNCg0K

Re: [PATCH 2/3 v2] iommu/fsl: Enable default DMA window for PCIe devices

[Alex Williamson]

On Tue, 2013-12-03 at 18:15 +0000, Varun Sethi wrote:
> 
> > -----Original Message-----
> > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > Sent: Tuesday, December 03, 2013 3:04 AM
> > To: Sethi Varun-B16395
> > Cc: joro@8bytes.org; iommu@lists.linux-foundation.org; linuxppc-
> > dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; Yoder Stuart-B08248;
> > Wood Scott-B07421; Bhushan Bharat-R65777
> > Subject: Re: [PATCH 2/3 v2] iommu/fsl: Enable default DMA window for PCIe
> > devices
> > 
> > On Wed, 2013-10-16 at 16:53 +0530, Varun Sethi wrote:
> > > Once the PCIe device assigned to a guest VM (via VFIO) gets detached
> > > from the iommu domain (when guest terminates), its PAMU table entry is
> > > disabled. So, this would prevent the device from being used once it's
> > assigned back to the host.
> > >
> > > This patch allows for creation of a default DMA window corresponding
> > > to the device and subsequently enabling the PAMU table entry. Before
> > > we enable the entry, we ensure that the device's bus master capability
> > is disabled (device quiesced).
> > >
> > > Signed-off-by: Varun Sethi <Varun.Sethi@freescale.com>
> > > ---
> > >  drivers/iommu/fsl_pamu.c        |   43 ++++++++++++++++++++++++++++---
> > -----
> > >  drivers/iommu/fsl_pamu.h        |    1 +
> > >  drivers/iommu/fsl_pamu_domain.c |   46
> > ++++++++++++++++++++++++++++++++++++---
> > >  3 files changed, 78 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/iommu/fsl_pamu.c b/drivers/iommu/fsl_pamu.c index
> > > cba0498..fb4a031 100644
> > > --- a/drivers/iommu/fsl_pamu.c
> > > +++ b/drivers/iommu/fsl_pamu.c
> > > @@ -225,6 +225,21 @@ static struct paace *pamu_get_spaace(struct paace
> > *paace, u32 wnum)
> > >  	return spaace;
> > >  }
> > >
> > > +/*
> > > + * Defaul PPAACE settings for an LIODN.
> > > + */
> > > +static void setup_default_ppaace(struct paace *ppaace) {
> > > +	pamu_init_ppaace(ppaace);
> > > +	/* window size is 2^(WSE+1) bytes */
> > > +	set_bf(ppaace->addr_bitfields, PPAACE_AF_WSE, 35);
> > > +	ppaace->wbah = 0;
> > > +	set_bf(ppaace->addr_bitfields, PPAACE_AF_WBAL, 0);
> > > +	set_bf(ppaace->impl_attr, PAACE_IA_ATM,
> > > +		PAACE_ATM_NO_XLATE);
> > > +	set_bf(ppaace->addr_bitfields, PAACE_AF_AP,
> > > +		PAACE_AP_PERMS_ALL);
> > > +}
> > >  /**
> > >   * pamu_get_fspi_and_allocate() - Allocates fspi index and reserves
> > subwindows
> > >   *                                required for primary PAACE in the
> > secondary
> > > @@ -253,6 +268,24 @@ static unsigned long
> > pamu_get_fspi_and_allocate(u32 subwin_cnt)
> > >  	return (spaace_addr - (unsigned long)spaact) / (sizeof(struct
> > > paace));  }
> > >
> > > +/* Reset the PAACE entry to the default state */ void
> > > +enable_default_dma_window(int liodn) {
> > > +	struct paace *ppaace;
> > > +
> > > +	ppaace = pamu_get_ppaace(liodn);
> > > +	if (!ppaace) {
> > > +		pr_debug("Invalid liodn entry\n");
> > > +		return;
> > > +	}
> > > +
> > > +	memset(ppaace, 0, sizeof(struct paace));
> > > +
> > > +	setup_default_ppaace(ppaace);
> > > +	mb();
> > > +	pamu_enable_liodn(liodn);
> > > +}
> > > +
> > >  /* Release the subwindows reserved for a particular LIODN */  void
> > > pamu_free_subwins(int liodn)  { @@ -752,15 +785,7 @@ static void
> > > __init setup_liodns(void)
> > >  				continue;
> > >  			}
> > >  			ppaace = pamu_get_ppaace(liodn);
> > > -			pamu_init_ppaace(ppaace);
> > > -			/* window size is 2^(WSE+1) bytes */
> > > -			set_bf(ppaace->addr_bitfields, PPAACE_AF_WSE, 35);
> > > -			ppaace->wbah = 0;
> > > -			set_bf(ppaace->addr_bitfields, PPAACE_AF_WBAL, 0);
> > > -			set_bf(ppaace->impl_attr, PAACE_IA_ATM,
> > > -				PAACE_ATM_NO_XLATE);
> > > -			set_bf(ppaace->addr_bitfields, PAACE_AF_AP,
> > > -				PAACE_AP_PERMS_ALL);
> > > +			setup_default_ppaace(ppaace);
> > >  			if (of_device_is_compatible(node, "fsl,qman-portal"))
> > >  				setup_qbman_paace(ppaace, QMAN_PORTAL_PAACE);
> > >  			if (of_device_is_compatible(node, "fsl,qman")) diff --
> > git
> > > a/drivers/iommu/fsl_pamu.h b/drivers/iommu/fsl_pamu.h index
> > > 8fc1a12..0edcbbbb 100644
> > > --- a/drivers/iommu/fsl_pamu.h
> > > +++ b/drivers/iommu/fsl_pamu.h
> > > @@ -406,5 +406,6 @@ void get_ome_index(u32 *omi_index, struct device
> > > *dev);  int  pamu_update_paace_stash(int liodn, u32 subwin, u32
> > > value);  int pamu_disable_spaace(int liodn, u32 subwin);
> > >  u32 pamu_get_max_subwin_cnt(void);
> > > +void enable_default_dma_window(int liodn);
> > >
> > >  #endif  /* __FSL_PAMU_H */
> > > diff --git a/drivers/iommu/fsl_pamu_domain.c
> > > b/drivers/iommu/fsl_pamu_domain.c index 966ae70..dd6cafc 100644
> > > --- a/drivers/iommu/fsl_pamu_domain.c
> > > +++ b/drivers/iommu/fsl_pamu_domain.c
> > > @@ -340,17 +340,57 @@ static inline struct device_domain_info
> > *find_domain(struct device *dev)
> > >  	return dev->archdata.iommu_domain;
> > >  }
> > >
> > > +/* Disable device DMA capability and enable default DMA window */
> > > +static void disable_device_dma(struct device_domain_info *info,
> > > +				int enable_dma_window)
> > 
> > nit, bool?
> > 
> > > +{
> > > +#ifdef CONFIG_PCI
> > > +	if (info->dev->bus == &pci_bus_type) {
> > > +		struct pci_dev *pdev = NULL;
> > 
> > nit, unnecessary initialization
> > 
> > > +		pdev = to_pci_dev(info->dev);
> > > +		if (pci_is_enabled(pdev))
> > > +			pci_disable_device(pdev);
> > 
> > This looks suspicious.  What's the case where you're finding devices
> > where this is needed?  Logically the driver that called
> > pci_enable_device() should also call pci_disabled_device() when it's
> > done.  Maybe there's a case to be made that we can't rely on the driver,
> > but then maybe there should be a pr_debug/info here to notify that
> > somebody left the device running.  There might also be the question of
> > whether you should test the busmaster bit in the command register rather
> > than trusting these indirect paths if it's really for cleanup.  Thanks,
> > 
> Once the device is detached from the domain, we enable a default DMA
> window for the device. Before enabling the default window, I want to
> ensure that device DMA is disabled. VFIO subsystem handles this for
> vfio-pci. But, can we assume that this would always be the case?

That's what I'm asking.  It seems like pci_enable/disable_device is part
of the driver API and every enable should have a matching disable.  If
this is to catch cases where the disable never happened, then there
should be some indication that the previous driver did something wrong
and we should have some sort of printed warning.  Then there's also the
question of whether this fully does what you want it to.  This is an
indirect way of checking the busmaster bit in the command register.  If
you really want to make sure it's disabled, is it sufficient or should
we be reading the actual command register?  Thanks,

Alex