[PATCH 1/3] iommu: Disambiguate MSI region types

[PATCH 1/3] iommu: Disambiguate MSI region types

[Robin Murphy]

Whilst it doesn't matter much to VFIO at the moment, when parsing
reserved regions on the host side we really needs to be able to tell
the difference between the software-reserved region used to map MSIs
translated by an IOMMU, and hardware regions for which the write might
never even reach the IOMMU. In particular, ARM systems assume the former
topology, but may need to cope with the latter as well, which will
require rather different handling in the iommu-dma layer.

For clarity, rename the software-managed type to IOMMU_RESV_SW_MSI, use
IOMMU_RESV_MSI to describe the hardware type, and document everything a
little bit. Since the x86 MSI remapping hardware falls squarely under
this meaning of IOMMU_RESV_MSI, apply that type to their regions as well,
so that we tell a consistent story to userspace across platforms (and
have future consistency if those drivers start migrating to iommu-dma).

Fixes: d30ddcaa7b02 ("iommu: Add a new type field in iommu_resv_region")
CC: Eric Auger <eric.auger-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
CC: Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
CC: David Woodhouse <dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
CC: kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---
 drivers/iommu/amd_iommu.c       | 2 +-
 drivers/iommu/arm-smmu-v3.c     | 2 +-
 drivers/iommu/arm-smmu.c        | 2 +-
 drivers/iommu/intel-iommu.c     | 2 +-
 drivers/iommu/iommu.c           | 1 +
 drivers/vfio/vfio_iommu_type1.c | 2 +-
 include/linux/iommu.h           | 5 +++++
 7 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 98940d1392cb..b17536d6e69b 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -3202,7 +3202,7 @@ static void amd_iommu_get_resv_regions(struct device *dev,
 
 	region = iommu_alloc_resv_region(MSI_RANGE_START,
 					 MSI_RANGE_END - MSI_RANGE_START + 1,
-					 0, IOMMU_RESV_RESERVED);
+					 0, IOMMU_RESV_MSI);
 	if (!region)
 		return;
 	list_add_tail(&region->list, head);
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 5806a6acc94e..591bb96047c9 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -1888,7 +1888,7 @@ static void arm_smmu_get_resv_regions(struct device *dev,
 	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
 
 	region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
-					 prot, IOMMU_RESV_MSI);
+					 prot, IOMMU_RESV_SW_MSI);
 	if (!region)
 		return;
 
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index abf6496843a6..b493c99e17f7 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1608,7 +1608,7 @@ static void arm_smmu_get_resv_regions(struct device *dev,
 	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
 
 	region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
-					 prot, IOMMU_RESV_MSI);
+					 prot, IOMMU_RESV_SW_MSI);
 	if (!region)
 		return;
 
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 238ad3447712..f1611fd6f5b0 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -5249,7 +5249,7 @@ static void intel_iommu_get_resv_regions(struct device *device,
 
 	reg = iommu_alloc_resv_region(IOAPIC_RANGE_START,
 				      IOAPIC_RANGE_END - IOAPIC_RANGE_START + 1,
-				      0, IOMMU_RESV_RESERVED);
+				      0, IOMMU_RESV_MSI);
 	if (!reg)
 		return;
 	list_add_tail(&reg->list, head);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 8ea14f41a979..7dbc05f10d5a 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -72,6 +72,7 @@ static const char * const iommu_group_resv_type_string[] = {
 	[IOMMU_RESV_DIRECT]	= "direct",
 	[IOMMU_RESV_RESERVED]	= "reserved",
 	[IOMMU_RESV_MSI]	= "msi",
+	[IOMMU_RESV_SW_MSI]	= "msi",
 };
 
 #define IOMMU_GROUP_ATTR(_name, _mode, _show, _store)		\
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index c26fa1f3ed86..e32abdebd2df 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -1192,7 +1192,7 @@ static bool vfio_iommu_has_resv_msi(struct iommu_group *group,
 	INIT_LIST_HEAD(&group_resv_regions);
 	iommu_get_group_resv_regions(group, &group_resv_regions);
 	list_for_each_entry(region, &group_resv_regions, list) {
-		if (region->type & IOMMU_RESV_MSI) {
+		if (region->type & IOMMU_RESV_SW_MSI) {
 			*base = region->start;
 			ret = true;
 			goto out;
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 6a6de187ddc0..fad2c4913be4 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -125,9 +125,14 @@ enum iommu_attr {
 };
 
 /* These are the possible reserved region types */
+/* Memory regions which must have 1:1 translations present at all times */
 #define IOMMU_RESV_DIRECT	(1 << 0)
+/* Arbitrary "never map this or give it to a device" address ranges */
 #define IOMMU_RESV_RESERVED	(1 << 1)
+/* Hardware MSI region (untranslated) */
 #define IOMMU_RESV_MSI		(1 << 2)
+/* Software-managed MSI translation window */
+#define IOMMU_RESV_SW_MSI	(1 << 3)
 
 /**
  * struct iommu_resv_region - descriptor for a reserved memory region
-- 
2.11.0.dirty

[PATCH 2/3] iommu/dma: Don't reserve PCI I/O windows

[Robin Murphy]

Even if a host controller's CPU-side MMIO windows into PCI I/O space do
happen to leak into PCI memory space such that it might treat them as
peer addresses, trying to reserve the corresponding I/O space addresses
doesn't do anything to help solve that problem. Stop doing a silly thing.

Fixes: fade1ec055dc ("iommu/dma: Avoid PCI host bridge windows")
Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---
 drivers/iommu/dma-iommu.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 48d36ce59efb..1e0983488a8d 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -175,8 +175,7 @@ static void iova_reserve_pci_windows(struct pci_dev *dev,
 	unsigned long lo, hi;
 
 	resource_list_for_each_entry(window, &bridge->windows) {
-		if (resource_type(window->res) != IORESOURCE_MEM &&
-		    resource_type(window->res) != IORESOURCE_IO)
+		if (resource_type(window->res) != IORESOURCE_MEM)
 			continue;
 
 		lo = iova_pfn(iovad, window->res->start - window->offset);
-- 
2.11.0.dirty

[PATCH 3/3] iommu/dma: Handle IOMMU API reserved regions

[Robin Murphy]

Now that it's simple to discover the necessary reservations for a given
device/IOMMU combination, let's wire up the appropriate handling. Basic
reserved regions and direct-mapped regions are obvious enough to handle;
hardware MSI regions we can handle by pre-populating the appropriate
msi_pages in the cookie. That way, irqchip drivers which normally assume
MSIs to require mapping at the IOMMU can keep working without having
to special-case their iommu_dma_map_msi_msg() hook, or indeed be aware
at all of integration quirks preventing the IOMMU translating certain
addresses.

Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---
 drivers/iommu/dma-iommu.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 1e0983488a8d..1082ebf8a415 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -167,6 +167,69 @@ void iommu_put_dma_cookie(struct iommu_domain *domain)
 }
 EXPORT_SYMBOL(iommu_put_dma_cookie);
 
+static int cookie_init_hw_msi_region(struct iommu_dma_cookie *cookie,
+		phys_addr_t start, phys_addr_t end)
+{
+	struct iova_domain *iovad = &cookie->iovad;
+	struct iommu_dma_msi_page *msi_page;
+	int i, num_pages;
+
+	start &= ~iova_mask(iovad);
+	end = iova_align(iovad, end);
+	num_pages = (end - start) >> iova_shift(iovad);
+
+	msi_page = kcalloc(num_pages, sizeof(*msi_page), GFP_KERNEL);
+	if (!msi_page)
+		return -ENOMEM;
+
+	for (i = 0; i < num_pages; i++) {
+		msi_page[i].phys = start;
+		msi_page[i].iova = start;
+		INIT_LIST_HEAD(&msi_page[i].list);
+		list_add(&msi_page[i].list, &cookie->msi_page_list);
+		start += iovad->granule;
+	}
+
+	return 0;
+}
+
+static int iova_reserve_iommu_regions(struct device *dev,
+		struct iommu_domain *domain)
+{
+	struct iommu_dma_cookie *cookie = domain->iova_cookie;
+	struct iova_domain *iovad = &cookie->iovad;
+	struct iommu_resv_region *region;
+	struct list_head resv_regions;
+	unsigned long lo, hi;
+	int ret = 0;
+
+	INIT_LIST_HEAD(&resv_regions);
+	iommu_get_resv_regions(dev, &resv_regions);
+	list_for_each_entry(region, &resv_regions, list) {
+		/* We ARE the software that manages these! */
+		if (region->type & IOMMU_RESV_SW_MSI)
+			continue;
+
+		lo = iova_pfn(iovad, region->start);
+		hi = iova_pfn(iovad, region->start + region->length);
+		reserve_iova(iovad, lo, hi);
+
+		if (region->type & IOMMU_RESV_DIRECT) {
+			ret = iommu_map(domain, region->start, region->start,
+					region->length, region->prot);
+		} else if (region->type & IOMMU_RESV_MSI) {
+			ret = cookie_init_hw_msi_region(cookie, region->start,
+					region->start + region->length);
+		}
+
+		if (ret)
+			break;
+	}
+	iommu_put_resv_regions(dev, &resv_regions);
+
+	return ret;
+}
+
 static void iova_reserve_pci_windows(struct pci_dev *dev,
 		struct iova_domain *iovad)
 {
@@ -251,6 +314,8 @@ int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
 		init_iova_domain(iovad, 1UL << order, base_pfn, end_pfn);
 		if (pci)
 			iova_reserve_pci_windows(to_pci_dev(dev), iovad);
+		if (dev)
+			iova_reserve_iommu_regions(dev, domain);
 	}
 	return 0;
 }
-- 
2.11.0.dirty

RE: [PATCH 3/3] iommu/dma: Handle IOMMU API reserved regions

[Shameerali Kolothum Thodi]

Hi Robin,

> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy-5wv7dgnIgG8@public.gmane.org]
> Sent: Thursday, March 09, 2017 7:51 PM
> To: joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org
> Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; will.deacon-5wv7dgnIgG8@public.gmane.org;
> marc.zyngier-5wv7dgnIgG8@public.gmane.org; Gabriele Paoloni; John Garry; Shameerali Kolothum
> Thodi
> Subject: [PATCH 3/3] iommu/dma: Handle IOMMU API reserved regions
> 
> Now that it's simple to discover the necessary reservations for a given
> device/IOMMU combination, let's wire up the appropriate handling. Basic
> reserved regions and direct-mapped regions are obvious enough to
> handle; hardware MSI regions we can handle by pre-populating the
> appropriate msi_pages in the cookie. That way, irqchip drivers which
> normally assume MSIs to require mapping at the IOMMU can keep working
> without having to special-case their iommu_dma_map_msi_msg() hook, or
> indeed be aware at all of integration quirks preventing the IOMMU
> translating certain addresses.

Many thanks for this. This will help us to avoid the ugly ITS quirk to
support the D03/D05. I will go through this.

Cheers,
Thanks.

> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
>  drivers/iommu/dma-iommu.c | 65
> +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 1e0983488a8d..1082ebf8a415 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -167,6 +167,69 @@ void iommu_put_dma_cookie(struct iommu_domain
> *domain)  }  EXPORT_SYMBOL(iommu_put_dma_cookie);
[...]

RE: [PATCH 1/3] iommu: Disambiguate MSI region types

[Shameerali Kolothum Thodi]

Hi Robin,

> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy-5wv7dgnIgG8@public.gmane.org]
> Sent: Thursday, March 09, 2017 7:51 PM
> To: joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org
> Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; will.deacon-5wv7dgnIgG8@public.gmane.org;
> marc.zyngier-5wv7dgnIgG8@public.gmane.org; Gabriele Paoloni; John Garry; Shameerali Kolothum
> Thodi; Eric Auger; Alex Williamson; David Woodhouse;
> kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: [PATCH 1/3] iommu: Disambiguate MSI region types
> 
> Whilst it doesn't matter much to VFIO at the moment, when parsing
> reserved regions on the host side we really needs to be able to tell
> the difference between the software-reserved region used to map MSIs
> translated by an IOMMU, and hardware regions for which the write might
> never even reach the IOMMU. In particular, ARM systems assume the
> former topology, but may need to cope with the latter as well, which
> will require rather different handling in the iommu-dma layer.
> 
> For clarity, rename the software-managed type to IOMMU_RESV_SW_MSI, use
> IOMMU_RESV_MSI to describe the hardware type, and document everything a
> little bit. Since the x86 MSI remapping hardware falls squarely under
> this meaning of IOMMU_RESV_MSI, apply that type to their regions as
> well, so that we tell a consistent story to userspace across platforms
> (and have future consistency if those drivers start migrating to iommu-
> dma).
> 
> Fixes: d30ddcaa7b02 ("iommu: Add a new type field in
> iommu_resv_region")
> CC: Eric Auger <eric.auger-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> CC: Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> CC: David Woodhouse <dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
> CC: kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
>  drivers/iommu/amd_iommu.c       | 2 +-
>  drivers/iommu/arm-smmu-v3.c     | 2 +-
>  drivers/iommu/arm-smmu.c        | 2 +-
>  drivers/iommu/intel-iommu.c     | 2 +-
>  drivers/iommu/iommu.c           | 1 +
>  drivers/vfio/vfio_iommu_type1.c | 2 +-
>  include/linux/iommu.h           | 5 +++++
>  7 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 98940d1392cb..b17536d6e69b 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -3202,7 +3202,7 @@ static void amd_iommu_get_resv_regions(struct
> device *dev,
> 
>  	region = iommu_alloc_resv_region(MSI_RANGE_START,
>  					 MSI_RANGE_END - MSI_RANGE_START + 1,
> -					 0, IOMMU_RESV_RESERVED);
> +					 0, IOMMU_RESV_MSI);
>  	if (!region)
>  		return;
>  	list_add_tail(&region->list, head);
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 5806a6acc94e..591bb96047c9 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -1888,7 +1888,7 @@ static void arm_smmu_get_resv_regions(struct
> device *dev,
>  	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> 
>  	region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
> -					 prot, IOMMU_RESV_MSI);
> +					 prot, IOMMU_RESV_SW_MSI);

I have verified this patch series on D05 by modifying the smmuV3 code to 
reserve a HW_MSI_IOVA range corresponds to D05 ITS doorbell. It works. 
Thanks for these patches.

The next step looks like is to define a DT binding to get this in SMMU
Driver and also for ACPI possibly IORT node data updated to reflect the
HW MSI range if any. I will take a look into it and have discussions 
started to get this included in the IORT spec. Please let me know if you
have any other thoughts/ideas on this.

Many thanks,
Shameer

Re: [PATCH 3/3] iommu/dma: Handle IOMMU API reserved regions

[Auger Eric]

Hi Robin,

On 09/03/2017 20:50, Robin Murphy wrote:
> Now that it's simple to discover the necessary reservations for a given
> device/IOMMU combination, let's wire up the appropriate handling. Basic
> reserved regions and direct-mapped regions are obvious enough to handle;
> hardware MSI regions we can handle by pre-populating the appropriate
> msi_pages in the cookie. That way, irqchip drivers which normally assume
> MSIs to require mapping at the IOMMU can keep working without having
> to special-case their iommu_dma_map_msi_msg() hook, or indeed be aware
> at all of integration quirks preventing the IOMMU translating certain
> addresses.
> 
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
>  drivers/iommu/dma-iommu.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 1e0983488a8d..1082ebf8a415 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -167,6 +167,69 @@ void iommu_put_dma_cookie(struct iommu_domain *domain)
>  }
>  EXPORT_SYMBOL(iommu_put_dma_cookie);
>  
> +static int cookie_init_hw_msi_region(struct iommu_dma_cookie *cookie,
> +		phys_addr_t start, phys_addr_t end)
> +{
> +	struct iova_domain *iovad = &cookie->iovad;
> +	struct iommu_dma_msi_page *msi_page;
> +	int i, num_pages;
> +
> +	start &= ~iova_mask(iovad);
> +	end = iova_align(iovad, end);
Is it always safe if second argument is a phys_addr_t?
> +	num_pages = (end - start) >> iova_shift(iovad);
> +
> +	msi_page = kcalloc(num_pages, sizeof(*msi_page), GFP_KERNEL);
> +	if (!msi_page)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < num_pages; i++) {
> +		msi_page[i].phys = start;
> +		msi_page[i].iova = start;
> +		INIT_LIST_HEAD(&msi_page[i].list);
> +		list_add(&msi_page[i].list, &cookie->msi_page_list);
> +		start += iovad->granule;
> +	}
> +
> +	return 0;
> +}
> +
> +static int iova_reserve_iommu_regions(struct device *dev,
> +		struct iommu_domain *domain)
> +{
> +	struct iommu_dma_cookie *cookie = domain->iova_cookie;
> +	struct iova_domain *iovad = &cookie->iovad;
> +	struct iommu_resv_region *region;
> +	struct list_head resv_regions;
> +	unsigned long lo, hi;
> +	int ret = 0;
> +
> +	INIT_LIST_HEAD(&resv_regions);
> +	iommu_get_resv_regions(dev, &resv_regions);
> +	list_for_each_entry(region, &resv_regions, list) {
> +		/* We ARE the software that manages these! */
> +		if (region->type & IOMMU_RESV_SW_MSI)
> +			continue;
> +
> +		lo = iova_pfn(iovad, region->start);
> +		hi = iova_pfn(iovad, region->start + region->length);
> +		reserve_iova(iovad, lo, hi);
> +
> +		if (region->type & IOMMU_RESV_DIRECT) {
> +			ret = iommu_map(domain, region->start, region->start,
> +					region->length, region->prot);

in iommu.c, iommu_group_create_direct_mappings also iommu_map() direct
regions in some cases. Just to make sure cases don't overlap here.


> +		} else if (region->type & IOMMU_RESV_MSI) {
> +			ret = cookie_init_hw_msi_region(cookie, region->start,
> +					region->start + region->length);
> +		}
> +
> +		if (ret)
> +			break;
> +	}
> +	iommu_put_resv_regions(dev, &resv_regions);
> +
> +	return ret;
> +}
> +
>  static void iova_reserve_pci_windows(struct pci_dev *dev,
>  		struct iova_domain *iovad)
>  {
> @@ -251,6 +314,8 @@ int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
>  		init_iova_domain(iovad, 1UL << order, base_pfn, end_pfn);
>  		if (pci)
>  			iova_reserve_pci_windows(to_pci_dev(dev), iovad);
> +		if (dev)
> +			iova_reserve_iommu_regions(dev, domain);
Don't you want to escalate the returned value?

Besides
Reviewed-by: Eric Auger <eric.auger-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Thanks

Eric
>  	}
>  	return 0;
>  }
> 

Re: [PATCH 2/3] iommu/dma: Don't reserve PCI I/O windows

[Auger Eric]

Hi,

On 09/03/2017 20:50, Robin Murphy wrote:
> Even if a host controller's CPU-side MMIO windows into PCI I/O space do
> happen to leak into PCI memory space such that it might treat them as
> peer addresses, trying to reserve the corresponding I/O space addresses
> doesn't do anything to help solve that problem. Stop doing a silly thing.
> 
> Fixes: fade1ec055dc ("iommu/dma: Avoid PCI host bridge windows")
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
Reviewed-by: Eric Auger <eric.auger-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Regards

Eric
> ---
>  drivers/iommu/dma-iommu.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 48d36ce59efb..1e0983488a8d 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -175,8 +175,7 @@ static void iova_reserve_pci_windows(struct pci_dev *dev,
>  	unsigned long lo, hi;
>  
>  	resource_list_for_each_entry(window, &bridge->windows) {
> -		if (resource_type(window->res) != IORESOURCE_MEM &&
> -		    resource_type(window->res) != IORESOURCE_IO)
> +		if (resource_type(window->res) != IORESOURCE_MEM)
>  			continue;
>  
>  		lo = iova_pfn(iovad, window->res->start - window->offset);
> 

Re: [PATCH 1/3] iommu: Disambiguate MSI region types

[Auger Eric]

Hi Robin,

On 09/03/2017 20:50, Robin Murphy wrote:
> Whilst it doesn't matter much to VFIO at the moment, when parsing
> reserved regions on the host side we really needs to be able to tell
s/needs/need
> the difference between the software-reserved region used to map MSIs
> translated by an IOMMU, and hardware regions for which the write might
> never even reach the IOMMU. In particular, ARM systems assume the former
> topology, but may need to cope with the latter as well, which will
> require rather different handling in the iommu-dma layer.
> 
> For clarity, rename the software-managed type to IOMMU_RESV_SW_MSI, use
> IOMMU_RESV_MSI to describe the hardware type, and document everything a
> little bit. Since the x86 MSI remapping hardware falls squarely under
> this meaning of IOMMU_RESV_MSI, apply that type to their regions as well,
> so that we tell a consistent story to userspace across platforms (and
> have future consistency if those drivers start migrating to iommu-dma).
> 
> Fixes: d30ddcaa7b02 ("iommu: Add a new type field in iommu_resv_region")
does it really fall under the category of fix here?
> CC: Eric Auger <eric.auger@redhat.com>
> CC: Alex Williamson <alex.williamson@redhat.com>
> CC: David Woodhouse <dwmw2@infradead.org>
> CC: kvm@vger.kernel.org
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/iommu/amd_iommu.c       | 2 +-
>  drivers/iommu/arm-smmu-v3.c     | 2 +-
>  drivers/iommu/arm-smmu.c        | 2 +-
>  drivers/iommu/intel-iommu.c     | 2 +-
>  drivers/iommu/iommu.c           | 1 +
>  drivers/vfio/vfio_iommu_type1.c | 2 +-
>  include/linux/iommu.h           | 5 +++++
>  7 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 98940d1392cb..b17536d6e69b 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -3202,7 +3202,7 @@ static void amd_iommu_get_resv_regions(struct device *dev,
>  
>  	region = iommu_alloc_resv_region(MSI_RANGE_START,
>  					 MSI_RANGE_END - MSI_RANGE_START + 1,
> -					 0, IOMMU_RESV_RESERVED);
> +					 0, IOMMU_RESV_MSI);
>  	if (!region)
>  		return;
>  	list_add_tail(&region->list, head);
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 5806a6acc94e..591bb96047c9 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -1888,7 +1888,7 @@ static void arm_smmu_get_resv_regions(struct device *dev,
>  	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
>  
>  	region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
> -					 prot, IOMMU_RESV_MSI);
> +					 prot, IOMMU_RESV_SW_MSI);
>  	if (!region)
>  		return;
>  
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index abf6496843a6..b493c99e17f7 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1608,7 +1608,7 @@ static void arm_smmu_get_resv_regions(struct device *dev,
>  	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
>  
>  	region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
> -					 prot, IOMMU_RESV_MSI);
> +					 prot, IOMMU_RESV_SW_MSI);
>  	if (!region)
>  		return;
>  
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 238ad3447712..f1611fd6f5b0 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -5249,7 +5249,7 @@ static void intel_iommu_get_resv_regions(struct device *device,
>  
>  	reg = iommu_alloc_resv_region(IOAPIC_RANGE_START,
>  				      IOAPIC_RANGE_END - IOAPIC_RANGE_START + 1,
> -				      0, IOMMU_RESV_RESERVED);
> +				      0, IOMMU_RESV_MSI);
>  	if (!reg)
>  		return;
>  	list_add_tail(&reg->list, head);
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 8ea14f41a979..7dbc05f10d5a 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -72,6 +72,7 @@ static const char * const iommu_group_resv_type_string[] = {
>  	[IOMMU_RESV_DIRECT]	= "direct",
>  	[IOMMU_RESV_RESERVED]	= "reserved",
>  	[IOMMU_RESV_MSI]	= "msi",
> +	[IOMMU_RESV_SW_MSI]	= "msi",
>  };
>  
>  #define IOMMU_GROUP_ATTR(_name, _mode, _show, _store)		\
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index c26fa1f3ed86..e32abdebd2df 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -1192,7 +1192,7 @@ static bool vfio_iommu_has_resv_msi(struct iommu_group *group,
Maybe we should change the name of the function into
vfio_iommu_has_resv_sw_msi?

Besides
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric


>  	INIT_LIST_HEAD(&group_resv_regions);
>  	iommu_get_group_resv_regions(group, &group_resv_regions);
>  	list_for_each_entry(region, &group_resv_regions, list) {
> -		if (region->type & IOMMU_RESV_MSI) {
> +		if (region->type & IOMMU_RESV_SW_MSI) {
>  			*base = region->start;
>  			ret = true;
>  			goto out;
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 6a6de187ddc0..fad2c4913be4 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -125,9 +125,14 @@ enum iommu_attr {
>  };
>  
>  /* These are the possible reserved region types */
> +/* Memory regions which must have 1:1 translations present at all times */
>  #define IOMMU_RESV_DIRECT	(1 << 0)
> +/* Arbitrary "never map this or give it to a device" address ranges */
>  #define IOMMU_RESV_RESERVED	(1 << 1)
> +/* Hardware MSI region (untranslated) */
>  #define IOMMU_RESV_MSI		(1 << 2)
> +/* Software-managed MSI translation window */
> +#define IOMMU_RESV_SW_MSI	(1 << 3)
>  
>  /**
>   * struct iommu_resv_region - descriptor for a reserved memory region
> 

Re: [PATCH 3/3] iommu/dma: Handle IOMMU API reserved regions

[Robin Murphy]

Hi Eric,

On 13/03/17 13:07, Auger Eric wrote:
> Hi Robin,
> 
> On 09/03/2017 20:50, Robin Murphy wrote:
>> Now that it's simple to discover the necessary reservations for a given
>> device/IOMMU combination, let's wire up the appropriate handling. Basic
>> reserved regions and direct-mapped regions are obvious enough to handle;
>> hardware MSI regions we can handle by pre-populating the appropriate
>> msi_pages in the cookie. That way, irqchip drivers which normally assume
>> MSIs to require mapping at the IOMMU can keep working without having
>> to special-case their iommu_dma_map_msi_msg() hook, or indeed be aware
>> at all of integration quirks preventing the IOMMU translating certain
>> addresses.
>>
>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> ---
>>  drivers/iommu/dma-iommu.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 65 insertions(+)
>>
>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>> index 1e0983488a8d..1082ebf8a415 100644
>> --- a/drivers/iommu/dma-iommu.c
>> +++ b/drivers/iommu/dma-iommu.c
>> @@ -167,6 +167,69 @@ void iommu_put_dma_cookie(struct iommu_domain *domain)
>>  }
>>  EXPORT_SYMBOL(iommu_put_dma_cookie);
>>  
>> +static int cookie_init_hw_msi_region(struct iommu_dma_cookie *cookie,
>> +		phys_addr_t start, phys_addr_t end)
>> +{
>> +	struct iova_domain *iovad = &cookie->iovad;
>> +	struct iommu_dma_msi_page *msi_page;
>> +	int i, num_pages;
>> +
>> +	start &= ~iova_mask(iovad);
>> +	end = iova_align(iovad, end);
> Is it always safe if second argument is a phys_addr_t?

Ooh, I think you're right - for the corner case of 32-bit unsigned long
and a crazy system with a doorbell above 4GB, end would get truncated
too early. I'll rework the arithmetic here to be safer.

>> +	num_pages = (end - start) >> iova_shift(iovad);
>> +
>> +	msi_page = kcalloc(num_pages, sizeof(*msi_page), GFP_KERNEL);
>> +	if (!msi_page)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < num_pages; i++) {
>> +		msi_page[i].phys = start;
>> +		msi_page[i].iova = start;
>> +		INIT_LIST_HEAD(&msi_page[i].list);
>> +		list_add(&msi_page[i].list, &cookie->msi_page_list);
>> +		start += iovad->granule;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int iova_reserve_iommu_regions(struct device *dev,
>> +		struct iommu_domain *domain)
>> +{
>> +	struct iommu_dma_cookie *cookie = domain->iova_cookie;
>> +	struct iova_domain *iovad = &cookie->iovad;
>> +	struct iommu_resv_region *region;
>> +	struct list_head resv_regions;
>> +	unsigned long lo, hi;
>> +	int ret = 0;
>> +
>> +	INIT_LIST_HEAD(&resv_regions);
>> +	iommu_get_resv_regions(dev, &resv_regions);
>> +	list_for_each_entry(region, &resv_regions, list) {
>> +		/* We ARE the software that manages these! */
>> +		if (region->type & IOMMU_RESV_SW_MSI)
>> +			continue;
>> +
>> +		lo = iova_pfn(iovad, region->start);
>> +		hi = iova_pfn(iovad, region->start + region->length);
>> +		reserve_iova(iovad, lo, hi);
>> +
>> +		if (region->type & IOMMU_RESV_DIRECT) {
>> +			ret = iommu_map(domain, region->start, region->start,
>> +					region->length, region->prot);
> 
> in iommu.c, iommu_group_create_direct_mappings also iommu_map() direct
> regions in some cases. Just to make sure cases don't overlap here.

Ah, I had indeed managed to overlook that, thanks for the reminder. We
should only get here long after iommu_group_add_device(), so I think it
should be safe to assume that any direct regions are already mapped and
just reserve the IOVAs here.

>> +		} else if (region->type & IOMMU_RESV_MSI) {
>> +			ret = cookie_init_hw_msi_region(cookie, region->start,
>> +					region->start + region->length);
>> +		}
>> +
>> +		if (ret)
>> +			break;
>> +	}
>> +	iommu_put_resv_regions(dev, &resv_regions);
>> +
>> +	return ret;
>> +}
>> +
>>  static void iova_reserve_pci_windows(struct pci_dev *dev,
>>  		struct iova_domain *iovad)
>>  {
>> @@ -251,6 +314,8 @@ int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
>>  		init_iova_domain(iovad, 1UL << order, base_pfn, end_pfn);
>>  		if (pci)
>>  			iova_reserve_pci_windows(to_pci_dev(dev), iovad);
>> +		if (dev)
>> +			iova_reserve_iommu_regions(dev, domain);
> Don't you want to escalate the returned value?

Yes, I'd posted this before I actually wrote the follow-on patch to make
proper reserved regions of the PCI windows as well (which I'll include
in v2) - that one does rearrange the return value logic here, but that's
no excuse for not doing it in this patch. Will fix.

> Besides
> Reviewed-by: Eric Auger <eric.auger-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Thanks!

Robin.

> 
> Thanks
> 
> Eric
>>  	}
>>  	return 0;
>>  }
>>

Re: [PATCH 1/3] iommu: Disambiguate MSI region types

[Robin Murphy]

On 13/03/17 13:08, Auger Eric wrote:
> Hi Robin,
> 
> On 09/03/2017 20:50, Robin Murphy wrote:
>> Whilst it doesn't matter much to VFIO at the moment, when parsing
>> reserved regions on the host side we really needs to be able to tell
> s/needs/need

Oops!

>> the difference between the software-reserved region used to map MSIs
>> translated by an IOMMU, and hardware regions for which the write might
>> never even reach the IOMMU. In particular, ARM systems assume the former
>> topology, but may need to cope with the latter as well, which will
>> require rather different handling in the iommu-dma layer.
>>
>> For clarity, rename the software-managed type to IOMMU_RESV_SW_MSI, use
>> IOMMU_RESV_MSI to describe the hardware type, and document everything a
>> little bit. Since the x86 MSI remapping hardware falls squarely under
>> this meaning of IOMMU_RESV_MSI, apply that type to their regions as well,
>> so that we tell a consistent story to userspace across platforms (and
>> have future consistency if those drivers start migrating to iommu-dma).
>>
>> Fixes: d30ddcaa7b02 ("iommu: Add a new type field in iommu_resv_region")
> does it really fall under the category of fix here?

I was somewhat on the fence about that, as the rationale above does tend
towards future new functionality, but the primary effect of this patch
alone is an ABI-visible change for x86 in terms of "expose the MSI
region as an MSI region". IMO it would be better to get that in before
said ABI gets baked into a kernel release, hence leaning towards the
"fix" side of things. I'm happy to rewrite the commit message in reverse
order (i.e. "clean up this ABI inconsistency, with these additional
benefits") if it would be clearer.

>> CC: Eric Auger <eric.auger-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> CC: Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> CC: David Woodhouse <dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
>> CC: kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> ---
>>  drivers/iommu/amd_iommu.c       | 2 +-
>>  drivers/iommu/arm-smmu-v3.c     | 2 +-
>>  drivers/iommu/arm-smmu.c        | 2 +-
>>  drivers/iommu/intel-iommu.c     | 2 +-
>>  drivers/iommu/iommu.c           | 1 +
>>  drivers/vfio/vfio_iommu_type1.c | 2 +-
>>  include/linux/iommu.h           | 5 +++++
>>  7 files changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
>> index 98940d1392cb..b17536d6e69b 100644
>> --- a/drivers/iommu/amd_iommu.c
>> +++ b/drivers/iommu/amd_iommu.c
>> @@ -3202,7 +3202,7 @@ static void amd_iommu_get_resv_regions(struct device *dev,
>>  
>>  	region = iommu_alloc_resv_region(MSI_RANGE_START,
>>  					 MSI_RANGE_END - MSI_RANGE_START + 1,
>> -					 0, IOMMU_RESV_RESERVED);
>> +					 0, IOMMU_RESV_MSI);
>>  	if (!region)
>>  		return;
>>  	list_add_tail(&region->list, head);
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index 5806a6acc94e..591bb96047c9 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -1888,7 +1888,7 @@ static void arm_smmu_get_resv_regions(struct device *dev,
>>  	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
>>  
>>  	region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
>> -					 prot, IOMMU_RESV_MSI);
>> +					 prot, IOMMU_RESV_SW_MSI);
>>  	if (!region)
>>  		return;
>>  
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index abf6496843a6..b493c99e17f7 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -1608,7 +1608,7 @@ static void arm_smmu_get_resv_regions(struct device *dev,
>>  	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
>>  
>>  	region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
>> -					 prot, IOMMU_RESV_MSI);
>> +					 prot, IOMMU_RESV_SW_MSI);
>>  	if (!region)
>>  		return;
>>  
>> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
>> index 238ad3447712..f1611fd6f5b0 100644
>> --- a/drivers/iommu/intel-iommu.c
>> +++ b/drivers/iommu/intel-iommu.c
>> @@ -5249,7 +5249,7 @@ static void intel_iommu_get_resv_regions(struct device *device,
>>  
>>  	reg = iommu_alloc_resv_region(IOAPIC_RANGE_START,
>>  				      IOAPIC_RANGE_END - IOAPIC_RANGE_START + 1,
>> -				      0, IOMMU_RESV_RESERVED);
>> +				      0, IOMMU_RESV_MSI);
>>  	if (!reg)
>>  		return;
>>  	list_add_tail(&reg->list, head);
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 8ea14f41a979..7dbc05f10d5a 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -72,6 +72,7 @@ static const char * const iommu_group_resv_type_string[] = {
>>  	[IOMMU_RESV_DIRECT]	= "direct",
>>  	[IOMMU_RESV_RESERVED]	= "reserved",
>>  	[IOMMU_RESV_MSI]	= "msi",
>> +	[IOMMU_RESV_SW_MSI]	= "msi",

As a side note, is it intentional that the values are bitfields, in
other words, is it ever going to make sense to combine them? The fact
that we use them directly to index an array here suggests not, and that
we might be better off with a plain enum. Otherwise, another possibility
would be to keep a single "MSI" type flag and add a separate
"software-managed" flag alongside it, with a corresponding tweak to
iommu_group_show_resv_regions(). Does anyone have a strong opinion
either way?

>>  };
>>  
>>  #define IOMMU_GROUP_ATTR(_name, _mode, _show, _store)		\
>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>> index c26fa1f3ed86..e32abdebd2df 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -1192,7 +1192,7 @@ static bool vfio_iommu_has_resv_msi(struct iommu_group *group,
> Maybe we should change the name of the function into
> vfio_iommu_has_resv_sw_msi?

Good idea, will do.

> Besides
> Reviewed-by: Eric Auger <eric.auger-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Thanks!

Robin.

> 
> Thanks
> 
> Eric
> 
> 
>>  	INIT_LIST_HEAD(&group_resv_regions);
>>  	iommu_get_group_resv_regions(group, &group_resv_regions);
>>  	list_for_each_entry(region, &group_resv_regions, list) {
>> -		if (region->type & IOMMU_RESV_MSI) {
>> +		if (region->type & IOMMU_RESV_SW_MSI) {
>>  			*base = region->start;
>>  			ret = true;
>>  			goto out;
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index 6a6de187ddc0..fad2c4913be4 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -125,9 +125,14 @@ enum iommu_attr {
>>  };
>>  
>>  /* These are the possible reserved region types */
>> +/* Memory regions which must have 1:1 translations present at all times */
>>  #define IOMMU_RESV_DIRECT	(1 << 0)
>> +/* Arbitrary "never map this or give it to a device" address ranges */
>>  #define IOMMU_RESV_RESERVED	(1 << 1)
>> +/* Hardware MSI region (untranslated) */
>>  #define IOMMU_RESV_MSI		(1 << 2)
>> +/* Software-managed MSI translation window */
>> +#define IOMMU_RESV_SW_MSI	(1 << 3)
>>  
>>  /**
>>   * struct iommu_resv_region - descriptor for a reserved memory region
>>

Re: [PATCH 1/3] iommu: Disambiguate MSI region types

[Auger Eric]

Hi Robin,

On 13/03/2017 15:24, Robin Murphy wrote:
> On 13/03/17 13:08, Auger Eric wrote:
>> Hi Robin,
>>
>> On 09/03/2017 20:50, Robin Murphy wrote:
>>> Whilst it doesn't matter much to VFIO at the moment, when parsing
>>> reserved regions on the host side we really needs to be able to tell
>> s/needs/need
> 
> Oops!
> 
>>> the difference between the software-reserved region used to map MSIs
>>> translated by an IOMMU, and hardware regions for which the write might
>>> never even reach the IOMMU. In particular, ARM systems assume the former
>>> topology, but may need to cope with the latter as well, which will
>>> require rather different handling in the iommu-dma layer.
>>>
>>> For clarity, rename the software-managed type to IOMMU_RESV_SW_MSI, use
>>> IOMMU_RESV_MSI to describe the hardware type, and document everything a
>>> little bit. Since the x86 MSI remapping hardware falls squarely under
>>> this meaning of IOMMU_RESV_MSI, apply that type to their regions as well,
>>> so that we tell a consistent story to userspace across platforms (and
>>> have future consistency if those drivers start migrating to iommu-dma).
>>>
>>> Fixes: d30ddcaa7b02 ("iommu: Add a new type field in iommu_resv_region")
>> does it really fall under the category of fix here?
> 
> I was somewhat on the fence about that, as the rationale above does tend
> towards future new functionality, but the primary effect of this patch
> alone is an ABI-visible change for x86 in terms of "expose the MSI
> region as an MSI region". IMO it would be better to get that in before
> said ABI gets baked into a kernel release, hence leaning towards the
> "fix" side of things. I'm happy to rewrite the commit message in reverse
> order (i.e. "clean up this ABI inconsistency, with these additional
> benefits") if it would be clearer.
OK no worries. I understand what you meant.
> 
>>> CC: Eric Auger <eric.auger@redhat.com>
>>> CC: Alex Williamson <alex.williamson@redhat.com>
>>> CC: David Woodhouse <dwmw2@infradead.org>
>>> CC: kvm@vger.kernel.org
>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>> ---
>>>  drivers/iommu/amd_iommu.c       | 2 +-
>>>  drivers/iommu/arm-smmu-v3.c     | 2 +-
>>>  drivers/iommu/arm-smmu.c        | 2 +-
>>>  drivers/iommu/intel-iommu.c     | 2 +-
>>>  drivers/iommu/iommu.c           | 1 +
>>>  drivers/vfio/vfio_iommu_type1.c | 2 +-
>>>  include/linux/iommu.h           | 5 +++++
>>>  7 files changed, 11 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
>>> index 98940d1392cb..b17536d6e69b 100644
>>> --- a/drivers/iommu/amd_iommu.c
>>> +++ b/drivers/iommu/amd_iommu.c
>>> @@ -3202,7 +3202,7 @@ static void amd_iommu_get_resv_regions(struct device *dev,
>>>  
>>>  	region = iommu_alloc_resv_region(MSI_RANGE_START,
>>>  					 MSI_RANGE_END - MSI_RANGE_START + 1,
>>> -					 0, IOMMU_RESV_RESERVED);
>>> +					 0, IOMMU_RESV_MSI);
>>>  	if (!region)
>>>  		return;
>>>  	list_add_tail(&region->list, head);
>>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>>> index 5806a6acc94e..591bb96047c9 100644
>>> --- a/drivers/iommu/arm-smmu-v3.c
>>> +++ b/drivers/iommu/arm-smmu-v3.c
>>> @@ -1888,7 +1888,7 @@ static void arm_smmu_get_resv_regions(struct device *dev,
>>>  	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
>>>  
>>>  	region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
>>> -					 prot, IOMMU_RESV_MSI);
>>> +					 prot, IOMMU_RESV_SW_MSI);
>>>  	if (!region)
>>>  		return;
>>>  
>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>>> index abf6496843a6..b493c99e17f7 100644
>>> --- a/drivers/iommu/arm-smmu.c
>>> +++ b/drivers/iommu/arm-smmu.c
>>> @@ -1608,7 +1608,7 @@ static void arm_smmu_get_resv_regions(struct device *dev,
>>>  	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
>>>  
>>>  	region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
>>> -					 prot, IOMMU_RESV_MSI);
>>> +					 prot, IOMMU_RESV_SW_MSI);
>>>  	if (!region)
>>>  		return;
>>>  
>>> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
>>> index 238ad3447712..f1611fd6f5b0 100644
>>> --- a/drivers/iommu/intel-iommu.c
>>> +++ b/drivers/iommu/intel-iommu.c
>>> @@ -5249,7 +5249,7 @@ static void intel_iommu_get_resv_regions(struct device *device,
>>>  
>>>  	reg = iommu_alloc_resv_region(IOAPIC_RANGE_START,
>>>  				      IOAPIC_RANGE_END - IOAPIC_RANGE_START + 1,
>>> -				      0, IOMMU_RESV_RESERVED);
>>> +				      0, IOMMU_RESV_MSI);
>>>  	if (!reg)
>>>  		return;
>>>  	list_add_tail(&reg->list, head);
>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>> index 8ea14f41a979..7dbc05f10d5a 100644
>>> --- a/drivers/iommu/iommu.c
>>> +++ b/drivers/iommu/iommu.c
>>> @@ -72,6 +72,7 @@ static const char * const iommu_group_resv_type_string[] = {
>>>  	[IOMMU_RESV_DIRECT]	= "direct",
>>>  	[IOMMU_RESV_RESERVED]	= "reserved",
>>>  	[IOMMU_RESV_MSI]	= "msi",
>>> +	[IOMMU_RESV_SW_MSI]	= "msi",
> 
> As a side note, is it intentional that the values are bitfields, in
> other words, is it ever going to make sense to combine them? The fact
> that we use them directly to index an array here suggests not, and that
> we might be better off with a plain enum. Otherwise, another possibility
> would be to keep a single "MSI" type flag and add a separate
> "software-managed" flag alongside it, with a corresponding tweak to
> iommu_group_show_resv_regions(). Does anyone have a strong opinion
> either way?

Originally this stems from "IOMMU_NOMAP" flavored define but I think
this should be transformed into an enum instead.

Thanks

Eric
> 
>>>  };
>>>  
>>>  #define IOMMU_GROUP_ATTR(_name, _mode, _show, _store)		\
>>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>>> index c26fa1f3ed86..e32abdebd2df 100644
>>> --- a/drivers/vfio/vfio_iommu_type1.c
>>> +++ b/drivers/vfio/vfio_iommu_type1.c
>>> @@ -1192,7 +1192,7 @@ static bool vfio_iommu_has_resv_msi(struct iommu_group *group,
>> Maybe we should change the name of the function into
>> vfio_iommu_has_resv_sw_msi?
> 
> Good idea, will do.
> 
>> Besides
>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> 
> Thanks!
> 
> Robin.
> 
>>
>> Thanks
>>
>> Eric
>>
>>
>>>  	INIT_LIST_HEAD(&group_resv_regions);
>>>  	iommu_get_group_resv_regions(group, &group_resv_regions);
>>>  	list_for_each_entry(region, &group_resv_regions, list) {
>>> -		if (region->type & IOMMU_RESV_MSI) {
>>> +		if (region->type & IOMMU_RESV_SW_MSI) {
>>>  			*base = region->start;
>>>  			ret = true;
>>>  			goto out;
>>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>>> index 6a6de187ddc0..fad2c4913be4 100644
>>> --- a/include/linux/iommu.h
>>> +++ b/include/linux/iommu.h
>>> @@ -125,9 +125,14 @@ enum iommu_attr {
>>>  };
>>>  
>>>  /* These are the possible reserved region types */
>>> +/* Memory regions which must have 1:1 translations present at all times */
>>>  #define IOMMU_RESV_DIRECT	(1 << 0)
>>> +/* Arbitrary "never map this or give it to a device" address ranges */
>>>  #define IOMMU_RESV_RESERVED	(1 << 1)
>>> +/* Hardware MSI region (untranslated) */
>>>  #define IOMMU_RESV_MSI		(1 << 2)
>>> +/* Software-managed MSI translation window */
>>> +#define IOMMU_RESV_SW_MSI	(1 << 3)
>>>  
>>>  /**
>>>   * struct iommu_resv_region - descriptor for a reserved memory region
>>>
>