[PATCH v2] intel_iommu,dmar: reserve mmio of IOMMU registers

[PATCH v2] intel_iommu,dmar: reserve mmio of IOMMU registers

[Donald Dutile]

Intel-iommu initialization doesn't currently reserve the memory used
for the IOMMU registers. This can allow the pci resource allocator
to assign a device BAR to the same address as the IOMMU registers.
This can cause some not so nice side affects when the driver
ioremap's that region.

Signed-off-by: Donald Dutile <ddutile-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

v2: missed a release-mem after an iounmap() in v1, which
    caused the additional changes to add the phys_addr
    and variable register region size to be stored in the
    intel_iommu struct
---
 drivers/iommu/dmar.c        |   35 +++++++++++++++++++++++++++--------
 include/linux/intel-iommu.h |    3 ++-
 2 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index 35c1e17..0747c7f 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -581,7 +581,7 @@ int __init detect_intel_iommu(void)
 int alloc_iommu(struct dmar_drhd_unit *drhd)
 {
 	struct intel_iommu *iommu;
-	int map_size;
+	resource_size_t map_size;
 	u32 ver;
 	static int iommu_allocated = 0;
 	int agaw = 0;
@@ -599,10 +599,17 @@ 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);
+	iommu->reg_phys = drhd->reg_base_addr;
+	iommu->reg_size = VTD_PAGE_SIZE;
+	if (!request_mem_region(iommu->reg_phys, iommu->reg_size, iommu->name)) {
+		printk(KERN_ERR "IOMMU: can't reserve memory\n");
+		goto error;
+	}
+
+	iommu->reg = ioremap(iommu->reg_phys, iommu->reg_size);
 	if (!iommu->reg) {
 		printk(KERN_ERR "IOMMU: can't map the region\n");
-		goto error;
+		goto err_release;
 	}
 	iommu->cap = dmar_readq(iommu->reg + DMAR_CAP_REG);
 	iommu->ecap = dmar_readq(iommu->reg + DMAR_ECAP_REG);
@@ -635,19 +642,26 @@ int alloc_iommu(struct dmar_drhd_unit *drhd)
 	map_size = max_t(int, ecap_max_iotlb_offset(iommu->ecap),
 		cap_max_fault_reg_offset(iommu->cap));
 	map_size = VTD_PAGE_ALIGN(map_size);
-	if (map_size > VTD_PAGE_SIZE) {
+	if (map_size > iommu->reg_size) {
 		iounmap(iommu->reg);
-		iommu->reg = ioremap(drhd->reg_base_addr, map_size);
+		release_mem_region(iommu->reg_phys, iommu->reg_size);
+		iommu->reg_size = map_size;
+		if (!request_mem_region(iommu->reg_phys, iommu->reg_size, 
+					iommu->name)) {
+			printk(KERN_ERR "IOMMU: can't reserve memory\n");
+			goto error;
+		}
+		iommu->reg = ioremap(iommu->reg_phys, iommu->reg_size);
 		if (!iommu->reg) {
 			printk(KERN_ERR "IOMMU: can't map the region\n");
-			goto error;
+			goto err_release;
 		}
 	}
 
 	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,
+		(unsigned long long)iommu->reg_phys,
 		DMAR_VER_MAJOR(ver), DMAR_VER_MINOR(ver),
 		(unsigned long long)iommu->cap,
 		(unsigned long long)iommu->ecap);
@@ -659,6 +673,8 @@ int alloc_iommu(struct dmar_drhd_unit *drhd)
 
  err_unmap:
 	iounmap(iommu->reg);
+ err_release:
+	release_mem_region(iommu->reg_phys, iommu->reg_size);
  error:
 	kfree(iommu);
 	return -1;
@@ -671,8 +687,11 @@ 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_phys, iommu->reg_size);
+	}
+
 	kfree(iommu);
 }
 
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index e6ca56d..c6d132b 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 */
+	resource_size_t reg_phys;
+	resource_size_t reg_size;
 	u64		cap;
 	u64		ecap;
 	u32		gcmd; /* Holds TE, EAFL. Don't need SRTP, SFL, WBF */
@@ -360,7 +362,6 @@ extern void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16 qdep,
 			       u64 addr, unsigned mask);
 
 extern int qi_submit_sync(struct qi_desc *desc, struct intel_iommu *iommu);
-
 extern int dmar_ir_support(void);
 
 #endif
-- 
1.7.1

Re: [PATCH v2] intel_iommu,dmar: reserve mmio of IOMMU registers

[Chris Wright]

* Donald Dutile (ddutile-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org) wrote:
> --- a/drivers/iommu/dmar.c
> +++ b/drivers/iommu/dmar.c
> @@ -581,7 +581,7 @@ int __init detect_intel_iommu(void)
>  int alloc_iommu(struct dmar_drhd_unit *drhd)
>  {
>  	struct intel_iommu *iommu;
> -	int map_size;
> +	resource_size_t map_size;
>  	u32 ver;
>  	static int iommu_allocated = 0;
>  	int agaw = 0;
> @@ -599,10 +599,17 @@ 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);

I think it'd be nice to create a helper function that does the request
and map.  This should include the map, read cap/ecap, calculate size and
possbily remap.  Would probably simplify the error cleanup too.

> +	iommu->reg_phys = drhd->reg_base_addr;
> +	iommu->reg_size = VTD_PAGE_SIZE;
> +	if (!request_mem_region(iommu->reg_phys, iommu->reg_size, iommu->name)) {
> +		printk(KERN_ERR "IOMMU: can't reserve memory\n");
> +		goto error;
> +	}
> +
> +	iommu->reg = ioremap(iommu->reg_phys, iommu->reg_size);
>  	if (!iommu->reg) {
>  		printk(KERN_ERR "IOMMU: can't map the region\n");
> -		goto error;
> +		goto err_release;
>  	}
>  	iommu->cap = dmar_readq(iommu->reg + DMAR_CAP_REG);
>  	iommu->ecap = dmar_readq(iommu->reg + DMAR_ECAP_REG);
> @@ -635,19 +642,26 @@ int alloc_iommu(struct dmar_drhd_unit *drhd)
>  	map_size = max_t(int, ecap_max_iotlb_offset(iommu->ecap),
>  		cap_max_fault_reg_offset(iommu->cap));
>  	map_size = VTD_PAGE_ALIGN(map_size);
> -	if (map_size > VTD_PAGE_SIZE) {
> +	if (map_size > iommu->reg_size) {
>  		iounmap(iommu->reg);
> -		iommu->reg = ioremap(drhd->reg_base_addr, map_size);
> +		release_mem_region(iommu->reg_phys, iommu->reg_size);
> +		iommu->reg_size = map_size;
> +		if (!request_mem_region(iommu->reg_phys, iommu->reg_size, 
> +					iommu->name)) {
> +			printk(KERN_ERR "IOMMU: can't reserve memory\n");
> +			goto error;
> +		}
> +		iommu->reg = ioremap(iommu->reg_phys, iommu->reg_size);
>  		if (!iommu->reg) {
>  			printk(KERN_ERR "IOMMU: can't map the region\n");
> -			goto error;
> +			goto err_release;
>  		}
>  	}
>  
>  	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,
> +		(unsigned long long)iommu->reg_phys,
>  		DMAR_VER_MAJOR(ver), DMAR_VER_MINOR(ver),
>  		(unsigned long long)iommu->cap,
>  		(unsigned long long)iommu->ecap);
> @@ -659,6 +673,8 @@ int alloc_iommu(struct dmar_drhd_unit *drhd)
>  
>   err_unmap:
>  	iounmap(iommu->reg);
> + err_release:
> +	release_mem_region(iommu->reg_phys, iommu->reg_size);
>   error:
>  	kfree(iommu);
>  	return -1;
> @@ -671,8 +687,11 @@ 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_phys, iommu->reg_size);
> +	}
> +
>  	kfree(iommu);
>  }
>  
> diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
> index e6ca56d..c6d132b 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 */
> +	resource_size_t reg_phys;
> +	resource_size_t reg_size;

I'd make these u64

Re: [PATCH v2] intel_iommu,dmar: reserve mmio of IOMMU registers

[Don Dutile]

On 04/05/2012 04:58 PM, Chris Wright wrote:
> * Donald Dutile (ddutile-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org) wrote:
>> --- a/drivers/iommu/dmar.c
>> +++ b/drivers/iommu/dmar.c
>> @@ -581,7 +581,7 @@ int __init detect_intel_iommu(void)
>>   int alloc_iommu(struct dmar_drhd_unit *drhd)
>>   {
>>   	struct intel_iommu *iommu;
>> -	int map_size;
>> +	resource_size_t map_size;
>>   	u32 ver;
>>   	static int iommu_allocated = 0;
>>   	int agaw = 0;
>> @@ -599,10 +599,17 @@ 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);
>
> I think it'd be nice to create a helper function that does the request
> and map.  This should include the map, read cap/ecap, calculate size and
> possbily remap.  Would probably simplify the error cleanup too.
>
>> +	iommu->reg_phys = drhd->reg_base_addr;
>> +	iommu->reg_size = VTD_PAGE_SIZE;
>> +	if (!request_mem_region(iommu->reg_phys, iommu->reg_size, iommu->name)) {
>> +		printk(KERN_ERR "IOMMU: can't reserve memory\n");
>> +		goto error;
>> +	}
>> +
>> +	iommu->reg = ioremap(iommu->reg_phys, iommu->reg_size);
>>   	if (!iommu->reg) {
>>   		printk(KERN_ERR "IOMMU: can't map the region\n");
>> -		goto error;
>> +		goto err_release;
>>   	}
>>   	iommu->cap = dmar_readq(iommu->reg + DMAR_CAP_REG);
>>   	iommu->ecap = dmar_readq(iommu->reg + DMAR_ECAP_REG);
>> @@ -635,19 +642,26 @@ int alloc_iommu(struct dmar_drhd_unit *drhd)
>>   	map_size = max_t(int, ecap_max_iotlb_offset(iommu->ecap),
>>   		cap_max_fault_reg_offset(iommu->cap));
>>   	map_size = VTD_PAGE_ALIGN(map_size);
>> -	if (map_size>  VTD_PAGE_SIZE) {
>> +	if (map_size>  iommu->reg_size) {
>>   		iounmap(iommu->reg);
>> -		iommu->reg = ioremap(drhd->reg_base_addr, map_size);
>> +		release_mem_region(iommu->reg_phys, iommu->reg_size);
>> +		iommu->reg_size = map_size;
>> +		if (!request_mem_region(iommu->reg_phys, iommu->reg_size,
>> +					iommu->name)) {
>> +			printk(KERN_ERR "IOMMU: can't reserve memory\n");
>> +			goto error;
>> +		}
>> +		iommu->reg = ioremap(iommu->reg_phys, iommu->reg_size);
>>   		if (!iommu->reg) {
>>   			printk(KERN_ERR "IOMMU: can't map the region\n");
>> -			goto error;
>> +			goto err_release;
>>   		}
>>   	}
>>
>>   	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,
>> +		(unsigned long long)iommu->reg_phys,
>>   		DMAR_VER_MAJOR(ver), DMAR_VER_MINOR(ver),
>>   		(unsigned long long)iommu->cap,
>>   		(unsigned long long)iommu->ecap);
>> @@ -659,6 +673,8 @@ int alloc_iommu(struct dmar_drhd_unit *drhd)
>>
>>    err_unmap:
>>   	iounmap(iommu->reg);
>> + err_release:
>> +	release_mem_region(iommu->reg_phys, iommu->reg_size);
>>    error:
>>   	kfree(iommu);
>>   	return -1;
>> @@ -671,8 +687,11 @@ 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_phys, iommu->reg_size);
>> +	}
>> +
>>   	kfree(iommu);
>>   }
>>
>> diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
>> index e6ca56d..c6d132b 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 */
>> +	resource_size_t reg_phys;
>> +	resource_size_t reg_size;
>
> I'd make these u64

ok, will work up v3 tomorrow..

thanks for feedback!