[PATCH 0/2] iommu: request CSR space

[PATCH 0/2] iommu: request CSR space

[Bjorn Helgaas]

This adds a request_mem_region() for the DRHD registers.  The AMD
code has similar code in iommu_map_mmio_space().

This helps avoid address conflicts when assigning PCI BARs.

---

Bjorn Helgaas (2):
      resources: set type of new resource returned by __request_region()
      iommu: Request IOMMU CSR space


 drivers/iommu/dmar.c        |   41 ++++++++++++++++++++++++++++++++---------
 include/linux/intel-iommu.h |    2 ++
 kernel/resource.c           |    3 ++-
 3 files changed, 36 insertions(+), 10 deletions(-)

[PATCH 1/2] resources: set type of new resource returned by __request_region()

[Bjorn Helgaas]

Previously we returned a new struct resource with only IORESOURCE_BUSY
set (and possibly IORESOURCE_MUXED or IORESOURCE_EXCLUSIVE), but no
MEM/IO/etc. bits set.  The new resource should inherit the type of
its parent.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 kernel/resource.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/kernel/resource.c b/kernel/resource.c
index 7e8ea66..04bb527 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -847,7 +847,8 @@ struct resource * __request_region(struct resource *parent,
 	res->name = name;
 	res->start = start;
 	res->end = start + n - 1;
-	res->flags = IORESOURCE_BUSY;
+	res->flags = parent->flags;
+	res->flags |= IORESOURCE_BUSY;
 	res->flags |= flags;
 
 	write_lock(&resource_lock);

Re: [PATCH 1/2] resources: set type of new resource returned by __request_region()

[Joerg Roedel]

On Fri, May 18, 2012 at 05:18:32PM -0600, Bjorn Helgaas wrote:
> Previously we returned a new struct resource with only IORESOURCE_BUSY
> set (and possibly IORESOURCE_MUXED or IORESOURCE_EXCLUSIVE), but no
> MEM/IO/etc. bits set.  The new resource should inherit the type of
> its parent.

Should it? What about IORESOURCE_WINDOW for example? Any particular
reason for this change to the interface of the __request_region()
function?


	Joerg

Re: [PATCH 1/2] resources: set type of new resource returned by __request_region()

[Bjorn Helgaas]

On Mon, May 21, 2012 at 7:04 AM, Joerg Roedel <joro@8bytes.org> wrote:
> On Fri, May 18, 2012 at 05:18:32PM -0600, Bjorn Helgaas wrote:
>> Previously we returned a new struct resource with only IORESOURCE_BUSY
>> set (and possibly IORESOURCE_MUXED or IORESOURCE_EXCLUSIVE), but no
>> MEM/IO/etc. bits set.  The new resource should inherit the type of
>> its parent.
>
> Should it? What about IORESOURCE_WINDOW for example? Any particular
> reason for this change to the interface of the __request_region()
> function?

Yes, I should have explained the connection :)  The next patch prints
the resource returned from request_mem_region().  Without this
__request_region() patch, that resource has no type, so %pR can't
print it correctly.

Good question about IORESOURCE_WINDOW.  Maybe it should inherit only
the IORESOURCE_TYPE_BITS part (IO/MEM/IRQ/DMA/BUS).  But it seemed
like it should also inherit PREFETCH, READONLY, CACHEABLE, etc.  Maybe
we should inherit everything but clear IORESOURCE_WINDOW?  I'm open to
suggestions here.

Bjorn

Re: [PATCH 1/2] resources: set type of new resource returned by __request_region()

[Joerg Roedel]

On Mon, May 21, 2012 at 07:52:24AM -0600, Bjorn Helgaas wrote:
> On Mon, May 21, 2012 at 7:04 AM, Joerg Roedel <joro@8bytes.org> wrote:
> > On Fri, May 18, 2012 at 05:18:32PM -0600, Bjorn Helgaas wrote:
> >> Previously we returned a new struct resource with only IORESOURCE_BUSY
> >> set (and possibly IORESOURCE_MUXED or IORESOURCE_EXCLUSIVE), but no
> >> MEM/IO/etc. bits set.  The new resource should inherit the type of
> >> its parent.
> >
> > Should it? What about IORESOURCE_WINDOW for example? Any particular
> > reason for this change to the interface of the __request_region()
> > function?
> 
> Yes, I should have explained the connection :)  The next patch prints
> the resource returned from request_mem_region().  Without this
> __request_region() patch, that resource has no type, so %pR can't
> print it correctly.
> 
> Good question about IORESOURCE_WINDOW.  Maybe it should inherit only
> the IORESOURCE_TYPE_BITS part (IO/MEM/IRQ/DMA/BUS).  But it seemed
> like it should also inherit PREFETCH, READONLY, CACHEABLE, etc.  Maybe
> we should inherit everything but clear IORESOURCE_WINDOW?  I'm open to
> suggestions here.

I am still not sure whether the inheritance is a good idea. How about
changing the define of request_mem_region to:

#define request_mem_region(start,n,name) __request_region(&iomem_resource, (start), (n), (name), IORESOURCE_MEM)

?

	Joerg

[PATCH 2/2] iommu: Request IOMMU CSR space

[Bjorn Helgaas]

Request the IOMMU CSR MMIO region to keep anybody else from claiming it
while we're using it.

The bug referenced below is a crash that happened when we assigned an
address in the IOMMU CSR area to a PCI BAR.  This patch just changes
the IOMMU driver, which only helps when the driver is present, so a
BIOS change is also necessary.  But the driver should claim the space
it uses in any event.

Reference: https://bugzilla.novell.com/show_bug.cgi?id=760440
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/iommu/dmar.c        |   41 ++++++++++++++++++++++++++++++++---------
 include/linux/intel-iommu.h |    2 ++
 2 files changed, 34 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index 35c1e17..e4ac23a 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -581,6 +581,7 @@ int __init detect_intel_iommu(void)
 int alloc_iommu(struct dmar_drhd_unit *drhd)
 {
 	struct intel_iommu *iommu;
+	struct resource *res;
 	int map_size;
 	u32 ver;
 	static int iommu_allocated = 0;
@@ -599,11 +600,20 @@ int alloc_iommu(struct dmar_drhd_unit *drhd)
 	iommu->seq_id = iommu_allocated++;
 	sprintf (iommu->name, "dmar%d", iommu->seq_id);
 
-	iommu->reg = ioremap(drhd->reg_base_addr, VTD_PAGE_SIZE);
-	if (!iommu->reg) {
-		printk(KERN_ERR "IOMMU: can't map the region\n");
+	iommu->reg_base = drhd->reg_base_addr;
+	iommu->reg_size = VTD_PAGE_SIZE;
+	res = request_mem_region(iommu->reg_base, iommu->reg_size, iommu->name);
+	if (!res) {
+		printk(KERN_ERR "IOMMU: can't request [mem %#010llx-%#010llx]\n",
+		       iommu->reg_base, iommu->reg_base + iommu->reg_size - 1);
 		goto error;
 	}
+
+	iommu->reg = ioremap(iommu->reg_base, iommu->reg_size);
+	if (!iommu->reg) {
+		printk(KERN_ERR "IOMMU: can't map %pR\n", res);
+		goto err_release;
+	}
 	iommu->cap = dmar_readq(iommu->reg + DMAR_CAP_REG);
 	iommu->ecap = dmar_readq(iommu->reg + DMAR_ECAP_REG);
 
@@ -637,17 +647,26 @@ int alloc_iommu(struct dmar_drhd_unit *drhd)
 	map_size = VTD_PAGE_ALIGN(map_size);
 	if (map_size > VTD_PAGE_SIZE) {
 		iounmap(iommu->reg);
-		iommu->reg = ioremap(drhd->reg_base_addr, map_size);
+		release_mem_region(iommu->reg_base, iommu->reg_size);
+		iommu->reg_size = map_size;
+		res = request_mem_region(iommu->reg_base, iommu->reg_size,
+					 iommu->name);
+		if (!res) {
+			printk(KERN_ERR "IOMMU: can't request [mem %#010llx-%#010llx]\n",
+				iommu->reg_base,
+				iommu->reg_base + iommu->reg_size - 1);
+			goto error;
+		}
+		iommu->reg = ioremap(iommu->reg_base, iommu->reg_size);
 		if (!iommu->reg) {
-			printk(KERN_ERR "IOMMU: can't map the region\n");
+			printk(KERN_ERR "IOMMU: can't map %pR\n", res);
 			goto error;
 		}
 	}
 
 	ver = readl(iommu->reg + DMAR_VER_REG);
-	pr_info("IOMMU %d: reg_base_addr %llx ver %d:%d cap %llx ecap %llx\n",
-		iommu->seq_id,
-		(unsigned long long)drhd->reg_base_addr,
+	pr_info("IOMMU %d: %pR ver %d:%d cap %llx ecap %llx\n",
+		iommu->seq_id, res,
 		DMAR_VER_MAJOR(ver), DMAR_VER_MINOR(ver),
 		(unsigned long long)iommu->cap,
 		(unsigned long long)iommu->ecap);
@@ -659,6 +678,8 @@ int alloc_iommu(struct dmar_drhd_unit *drhd)
 
  err_unmap:
 	iounmap(iommu->reg);
+ err_release:
+	release_mem_region(iommu->reg_base, iommu->reg_size);
  error:
 	kfree(iommu);
 	return -1;
@@ -671,8 +692,10 @@ void free_iommu(struct intel_iommu *iommu)
 
 	free_dmar_iommu(iommu);
 
-	if (iommu->reg)
+	if (iommu->reg) {
 		iounmap(iommu->reg);
+		release_mem_region(iommu->reg_base, iommu->reg_size);
+	}
 	kfree(iommu);
 }
 
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index e6ca56d..911ded0 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -308,6 +308,8 @@ enum {
 
 struct intel_iommu {
 	void __iomem	*reg; /* Pointer to hardware regs, virtual addr */
+	phys_addr_t	reg_base;
+	resource_size_t reg_size;
 	u64		cap;
 	u64		ecap;
 	u32		gcmd; /* Holds TE, EAFL. Don't need SRTP, SFL, WBF */

Re: [PATCH 2/2] iommu: Request IOMMU CSR space

[Joerg Roedel]

On Fri, May 18, 2012 at 05:18:37PM -0600, Bjorn Helgaas wrote:
> Request the IOMMU CSR MMIO region to keep anybody else from claiming it
> while we're using it.
> 
> The bug referenced below is a crash that happened when we assigned an
> address in the IOMMU CSR area to a PCI BAR.  This patch just changes
> the IOMMU driver, which only helps when the driver is present, so a
> BIOS change is also necessary.  But the driver should claim the space
> it uses in any event.
> 
> Reference: https://bugzilla.novell.com/show_bug.cgi?id=760440
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Looks good.

Reviewed-by: Joerg Roedel <joro@8bytes.org>

> ---
>  drivers/iommu/dmar.c        |   41 ++++++++++++++++++++++++++++++++---------
>  include/linux/intel-iommu.h |    2 ++
>  2 files changed, 34 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
> index 35c1e17..e4ac23a 100644
> --- a/drivers/iommu/dmar.c
> +++ b/drivers/iommu/dmar.c
> @@ -581,6 +581,7 @@ int __init detect_intel_iommu(void)
>  int alloc_iommu(struct dmar_drhd_unit *drhd)
>  {
>  	struct intel_iommu *iommu;
> +	struct resource *res;
>  	int map_size;
>  	u32 ver;
>  	static int iommu_allocated = 0;
> @@ -599,11 +600,20 @@ int alloc_iommu(struct dmar_drhd_unit *drhd)
>  	iommu->seq_id = iommu_allocated++;
>  	sprintf (iommu->name, "dmar%d", iommu->seq_id);
>  
> -	iommu->reg = ioremap(drhd->reg_base_addr, VTD_PAGE_SIZE);
> -	if (!iommu->reg) {
> -		printk(KERN_ERR "IOMMU: can't map the region\n");
> +	iommu->reg_base = drhd->reg_base_addr;
> +	iommu->reg_size = VTD_PAGE_SIZE;
> +	res = request_mem_region(iommu->reg_base, iommu->reg_size, iommu->name);
> +	if (!res) {
> +		printk(KERN_ERR "IOMMU: can't request [mem %#010llx-%#010llx]\n",
> +		       iommu->reg_base, iommu->reg_base + iommu->reg_size - 1);
>  		goto error;
>  	}
> +
> +	iommu->reg = ioremap(iommu->reg_base, iommu->reg_size);
> +	if (!iommu->reg) {
> +		printk(KERN_ERR "IOMMU: can't map %pR\n", res);
> +		goto err_release;
> +	}
>  	iommu->cap = dmar_readq(iommu->reg + DMAR_CAP_REG);
>  	iommu->ecap = dmar_readq(iommu->reg + DMAR_ECAP_REG);
>  
> @@ -637,17 +647,26 @@ int alloc_iommu(struct dmar_drhd_unit *drhd)
>  	map_size = VTD_PAGE_ALIGN(map_size);
>  	if (map_size > VTD_PAGE_SIZE) {
>  		iounmap(iommu->reg);
> -		iommu->reg = ioremap(drhd->reg_base_addr, map_size);
> +		release_mem_region(iommu->reg_base, iommu->reg_size);
> +		iommu->reg_size = map_size;
> +		res = request_mem_region(iommu->reg_base, iommu->reg_size,
> +					 iommu->name);
> +		if (!res) {
> +			printk(KERN_ERR "IOMMU: can't request [mem %#010llx-%#010llx]\n",
> +				iommu->reg_base,
> +				iommu->reg_base + iommu->reg_size - 1);
> +			goto error;
> +		}
> +		iommu->reg = ioremap(iommu->reg_base, iommu->reg_size);
>  		if (!iommu->reg) {
> -			printk(KERN_ERR "IOMMU: can't map the region\n");
> +			printk(KERN_ERR "IOMMU: can't map %pR\n", res);
>  			goto error;
>  		}
>  	}
>  
>  	ver = readl(iommu->reg + DMAR_VER_REG);
> -	pr_info("IOMMU %d: reg_base_addr %llx ver %d:%d cap %llx ecap %llx\n",
> -		iommu->seq_id,
> -		(unsigned long long)drhd->reg_base_addr,
> +	pr_info("IOMMU %d: %pR ver %d:%d cap %llx ecap %llx\n",
> +		iommu->seq_id, res,
>  		DMAR_VER_MAJOR(ver), DMAR_VER_MINOR(ver),
>  		(unsigned long long)iommu->cap,
>  		(unsigned long long)iommu->ecap);
> @@ -659,6 +678,8 @@ int alloc_iommu(struct dmar_drhd_unit *drhd)
>  
>   err_unmap:
>  	iounmap(iommu->reg);
> + err_release:
> +	release_mem_region(iommu->reg_base, iommu->reg_size);
>   error:
>  	kfree(iommu);
>  	return -1;
> @@ -671,8 +692,10 @@ void free_iommu(struct intel_iommu *iommu)
>  
>  	free_dmar_iommu(iommu);
>  
> -	if (iommu->reg)
> +	if (iommu->reg) {
>  		iounmap(iommu->reg);
> +		release_mem_region(iommu->reg_base, iommu->reg_size);
> +	}
>  	kfree(iommu);
>  }
>  
> diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
> index e6ca56d..911ded0 100644
> --- a/include/linux/intel-iommu.h
> +++ b/include/linux/intel-iommu.h
> @@ -308,6 +308,8 @@ enum {
>  
>  struct intel_iommu {
>  	void __iomem	*reg; /* Pointer to hardware regs, virtual addr */
> +	phys_addr_t	reg_base;
> +	resource_size_t reg_size;
>  	u64		cap;
>  	u64		ecap;
>  	u32		gcmd; /* Holds TE, EAFL. Don't need SRTP, SFL, WBF */
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/