[PATCH 06/13] add/remove domain device info for virtual machine VT-d

[PATCH 06/13] add/remove domain device info for virtual machine VT-d

[Han, Weidong]

[-- Attachment #1: Type: text/plain, Size: 7113 bytes --]

Separate add/remove domain device info functions for virtual machine VT-d from natvie VT-d.
 
Signed-off-by: Weidong Han <weidong.han@intel.com>
---
 drivers/pci/intel-iommu.c     |  164 +++++++++++++++++++++++++++++++++++++++-
 include/linux/dma_remapping.h |    1 +
 2 files changed, 160 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
index 09a5150..429aff4 100644
--- a/drivers/pci/intel-iommu.c
+++ b/drivers/pci/intel-iommu.c
@@ -200,6 +200,27 @@ static struct intel_iommu *domain_get_iommu(struct dmar_domain *domain)
 	return NULL;
 }
 
+static struct intel_iommu *device_find_matched_iommu(u8 bus, u8 devfn)
+{
+	struct dmar_drhd_unit *drhd = NULL;
+	int i;
+
+	for_each_drhd_unit(drhd) {
+		if (drhd->ignored)
+			continue;
+
+		for (i = 0; i < drhd->devices_cnt; i++)
+			if (drhd->devices[i]->bus->number == bus &&
+			    drhd->devices[i]->devfn == devfn)
+				return drhd->iommu;
+
+		if (drhd->include_all)
+			return drhd->iommu;
+	}
+
+	return NULL;
+}
+
 /* Gets context entry for a given bus and devfn */
 static struct context_entry * device_to_context_entry(struct intel_iommu *iommu,
 		u8 bus, u8 devfn)
@@ -934,7 +955,8 @@ void free_dmar_iommu(struct intel_iommu *iommu)
 	for (; i < cap_ndoms(iommu->cap); ) {
 		domain = iommu->domains[i];
 		clear_bit(i, iommu->domain_ids);
-		domain_exit(domain);
+		if (--domain->iommu_count == 0)
+			domain_exit(domain);
 		i = find_next_bit(iommu->domain_ids,
 			cap_ndoms(iommu->cap), i+1);
 	}
@@ -990,6 +1012,7 @@ static struct dmar_domain * iommu_alloc_domain(struct intel_iommu *iommu)
 
 	iommu->domains[num] = domain;
 	domain->flags = 0;
+	domain->iommu_count = 1;
 	spin_unlock_irqrestore(&iommu->lock, flags);
 
 	return domain;
@@ -1269,9 +1292,12 @@ domain_page_mapping(struct dmar_domain *domain, dma_addr_t iova,
 	return 0;
 }
 
-static void detach_domain_for_dev(struct dmar_domain *domain, u8 bus, u8 devfn)
+static void iommu_detach_dev(u8 bus, u8 devfn)
 {
-	struct intel_iommu *iommu = domain_get_iommu(domain);
+	struct intel_iommu *iommu = device_find_matched_iommu(bus, devfn);
+
+	if (!iommu)
+		return;
 
 	clear_context_table(iommu, bus, devfn);
 	iommu->flush.flush_context(iommu, 0, 0, 0,
@@ -1295,7 +1321,7 @@ static void domain_remove_dev_info(struct dmar_domain *domain)
 			info->dev->dev.archdata.iommu = NULL;
 		spin_unlock_irqrestore(&device_domain_lock, flags);
 
-		detach_domain_for_dev(info->domain, info->bus, info->devfn);
+		iommu_detach_dev(info->bus, info->devfn);
 		free_devinfo_mem(info);
 
 		spin_lock_irqsave(&device_domain_lock, flags);
@@ -2330,6 +2356,134 @@ int __init intel_iommu_init(void)
 	return 0;
 }
 
+/* "Coherency" capability may be different across iommus */
+static void domain_update_iommu_coherency(struct dmar_domain *domain)
+{
+	struct dmar_drhd_unit *drhd;
+
+	domain->iommu_coherency = 1;
+
+	for_each_drhd_unit(drhd) {
+		if (drhd->ignored)
+			continue;
+		if (test_bit(drhd->iommu->seq_id, &domain->iommu_bmp)) {
+			if (!ecap_coherent(drhd->iommu->ecap)) {
+				domain->iommu_coherency = 0;
+				break;
+			}
+		}
+	}
+}
+
+static int vm_domain_add_dev_info(struct dmar_domain *domain,
+				  struct pci_dev *pdev)
+{
+	struct device_domain_info *info;
+	unsigned long flags;
+
+	info = alloc_devinfo_mem();
+	if (!info)
+		return -ENOMEM;
+
+	info->bus = pdev->bus->number;
+	info->devfn = pdev->devfn;
+	info->dev = pdev;
+	info->domain = domain;
+
+	spin_lock_irqsave(&device_domain_lock, flags);
+	list_add(&info->link, &domain->devices);
+	list_add(&info->global, &device_domain_list);
+	pdev->dev.archdata.iommu = info;
+	spin_unlock_irqrestore(&device_domain_lock, flags);
+
+	return 0;
+}
+
+static void vm_domain_remove_one_dev_info(struct dmar_domain *domain,
+					  struct pci_dev *pdev)
+{
+	struct device_domain_info *info;
+	struct intel_iommu *iommu;
+	unsigned long flags;
+	int found = 0;
+
+	iommu = device_find_matched_iommu(pdev->bus->number, pdev->devfn);
+
+	spin_lock_irqsave(&device_domain_lock, flags);
+	while (!list_empty(&domain->devices)) {
+		info = list_entry(domain->devices.next,
+			struct device_domain_info, link);
+		if (info->bus == pdev->bus->number &&
+		    info->devfn == pdev->devfn) {
+			list_del(&info->link);
+			list_del(&info->global);
+			if (info->dev)
+				info->dev->dev.archdata.iommu = NULL;
+			spin_unlock_irqrestore(&device_domain_lock, flags);
+
+			iommu_detach_dev(info->bus, info->devfn);
+			free_devinfo_mem(info);
+
+			spin_lock_irqsave(&device_domain_lock, flags);
+
+			if (found)
+				break;
+			else
+				continue;
+		}
+
+		/* if there is no other devices under the same iommu
+		 * owned by this domain, clear this iommu in iommu_bmp
+		 */
+		if (device_find_matched_iommu(info->bus, info->devfn) == iommu)
+			found = 1;
+	}
+
+	if (found == 0) {
+		spin_lock_irqsave(&iommu->lock, flags);
+		clear_bit(iommu->seq_id, &domain->iommu_bmp);
+		domain->iommu_count--;
+		domain_update_iommu_coherency(domain);
+		spin_unlock_irqrestore(&iommu->lock, flags);
+	}
+
+	spin_unlock_irqrestore(&device_domain_lock, flags);
+}
+
+static void vm_domain_remove_all_dev_info(struct dmar_domain *domain)
+{
+	struct device_domain_info *info;
+	struct intel_iommu *iommu;
+	unsigned long flags;
+
+	spin_lock_irqsave(&device_domain_lock, flags);
+	while (!list_empty(&domain->devices)) {
+		info = list_entry(domain->devices.next,
+			struct device_domain_info, link);
+		list_del(&info->link);
+		list_del(&info->global);
+		if (info->dev)
+			info->dev->dev.archdata.iommu = NULL;
+
+		spin_unlock_irqrestore(&device_domain_lock, flags);
+		iommu_detach_dev(info->bus, info->devfn);
+
+		/* clear this iommu in iommu_bmp */
+		iommu = device_find_matched_iommu(info->bus, info->devfn);
+		spin_lock_irqsave(&iommu->lock, flags);
+		if (test_and_clear_bit(iommu->seq_id,
+				       &domain->iommu_bmp)) {
+			domain->iommu_count--;
+			domain_update_iommu_coherency(domain);
+		}
+		spin_unlock_irqrestore(&iommu->lock, flags);
+
+		free_devinfo_mem(info);
+		spin_lock_irqsave(&device_domain_lock, flags);
+	}
+	spin_unlock_irqrestore(&device_domain_lock, flags);
+}
+
 void intel_iommu_domain_exit(struct dmar_domain *domain)
 {
 	u64 end;
@@ -2407,7 +2561,7 @@ EXPORT_SYMBOL_GPL(intel_iommu_page_mapping);
 
 void intel_iommu_detach_dev(struct dmar_domain *domain, u8 bus, u8 devfn)
 {
-	detach_domain_for_dev(domain, bus, devfn);
+	iommu_detach_dev(bus, devfn);
 }
 EXPORT_SYMBOL_GPL(intel_iommu_detach_dev);
 
diff --git a/include/linux/dma_remapping.h b/include/linux/dma_remapping.h
index 9e39c99..f6925c5 100644
--- a/include/linux/dma_remapping.h
+++ b/include/linux/dma_remapping.h
@@ -186,6 +186,7 @@ struct dmar_domain {
 	int		flags;
 
 	int		iommu_coherency;/* iommu access is coherent or not */
+	int		iommu_count;	/* reference count of iommu */
 };
 
 /* PCI domain-device relationship */
-- 
1.5.1

[-- Attachment #2: 0006-add-remove-domain-device-info-for-virtual-machine.patch --]
[-- Type: application/octet-stream, Size: 7004 bytes --]

From 3c6f245c80d684afda4e5251eae00625f4f503da Mon Sep 17 00:00:00 2001
From: Weidong Han <weidong.han@intel.com>
Date: Tue, 2 Dec 2008 15:53:12 +0800
Subject: [PATCH] add/remove domain device info for virtual machine VT-d


Signed-off-by: Weidong Han <weidong.han@intel.com>
---
 drivers/pci/intel-iommu.c     |  164 +++++++++++++++++++++++++++++++++++++++-
 include/linux/dma_remapping.h |    1 +
 2 files changed, 160 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
index 09a5150..429aff4 100644
--- a/drivers/pci/intel-iommu.c
+++ b/drivers/pci/intel-iommu.c
@@ -200,6 +200,27 @@ static struct intel_iommu *domain_get_iommu(struct dmar_domain *domain)
 	return NULL;
 }
 
+static struct intel_iommu *device_find_matched_iommu(u8 bus, u8 devfn)
+{
+	struct dmar_drhd_unit *drhd = NULL;
+	int i;
+
+	for_each_drhd_unit(drhd) {
+		if (drhd->ignored)
+			continue;
+
+		for (i = 0; i < drhd->devices_cnt; i++)
+			if (drhd->devices[i]->bus->number == bus &&
+			    drhd->devices[i]->devfn == devfn)
+				return drhd->iommu;
+
+		if (drhd->include_all)
+			return drhd->iommu;
+	}
+
+	return NULL;
+}
+
 /* Gets context entry for a given bus and devfn */
 static struct context_entry * device_to_context_entry(struct intel_iommu *iommu,
 		u8 bus, u8 devfn)
@@ -934,7 +955,8 @@ void free_dmar_iommu(struct intel_iommu *iommu)
 	for (; i < cap_ndoms(iommu->cap); ) {
 		domain = iommu->domains[i];
 		clear_bit(i, iommu->domain_ids);
-		domain_exit(domain);
+		if (--domain->iommu_count == 0)
+			domain_exit(domain);
 		i = find_next_bit(iommu->domain_ids,
 			cap_ndoms(iommu->cap), i+1);
 	}
@@ -990,6 +1012,7 @@ static struct dmar_domain * iommu_alloc_domain(struct intel_iommu *iommu)
 
 	iommu->domains[num] = domain;
 	domain->flags = 0;
+	domain->iommu_count = 1;
 	spin_unlock_irqrestore(&iommu->lock, flags);
 
 	return domain;
@@ -1269,9 +1292,12 @@ domain_page_mapping(struct dmar_domain *domain, dma_addr_t iova,
 	return 0;
 }
 
-static void detach_domain_for_dev(struct dmar_domain *domain, u8 bus, u8 devfn)
+static void iommu_detach_dev(u8 bus, u8 devfn)
 {
-	struct intel_iommu *iommu = domain_get_iommu(domain);
+	struct intel_iommu *iommu = device_find_matched_iommu(bus, devfn);
+
+	if (!iommu)
+		return;
 
 	clear_context_table(iommu, bus, devfn);
 	iommu->flush.flush_context(iommu, 0, 0, 0,
@@ -1295,7 +1321,7 @@ static void domain_remove_dev_info(struct dmar_domain *domain)
 			info->dev->dev.archdata.iommu = NULL;
 		spin_unlock_irqrestore(&device_domain_lock, flags);
 
-		detach_domain_for_dev(info->domain, info->bus, info->devfn);
+		iommu_detach_dev(info->bus, info->devfn);
 		free_devinfo_mem(info);
 
 		spin_lock_irqsave(&device_domain_lock, flags);
@@ -2330,6 +2356,134 @@ int __init intel_iommu_init(void)
 	return 0;
 }
 
+/* "Coherency" capability may be different across iommus */
+static void domain_update_iommu_coherency(struct dmar_domain *domain)
+{
+	struct dmar_drhd_unit *drhd;
+
+	domain->iommu_coherency = 1;
+
+	for_each_drhd_unit(drhd) {
+		if (drhd->ignored)
+			continue;
+		if (test_bit(drhd->iommu->seq_id, &domain->iommu_bmp)) {
+			if (!ecap_coherent(drhd->iommu->ecap)) {
+				domain->iommu_coherency = 0;
+				break;
+			}
+		}
+	}
+}
+
+static int vm_domain_add_dev_info(struct dmar_domain *domain,
+				  struct pci_dev *pdev)
+{
+	struct device_domain_info *info;
+	unsigned long flags;
+
+	info = alloc_devinfo_mem();
+	if (!info)
+		return -ENOMEM;
+
+	info->bus = pdev->bus->number;
+	info->devfn = pdev->devfn;
+	info->dev = pdev;
+	info->domain = domain;
+
+	spin_lock_irqsave(&device_domain_lock, flags);
+	list_add(&info->link, &domain->devices);
+	list_add(&info->global, &device_domain_list);
+	pdev->dev.archdata.iommu = info;
+	spin_unlock_irqrestore(&device_domain_lock, flags);
+
+	return 0;
+}
+
+static void vm_domain_remove_one_dev_info(struct dmar_domain *domain,
+					  struct pci_dev *pdev)
+{
+	struct device_domain_info *info;
+	struct intel_iommu *iommu;
+	unsigned long flags;
+	int found = 0;
+
+	iommu = device_find_matched_iommu(pdev->bus->number, pdev->devfn);
+
+	spin_lock_irqsave(&device_domain_lock, flags);
+	while (!list_empty(&domain->devices)) {
+		info = list_entry(domain->devices.next,
+			struct device_domain_info, link);
+		if (info->bus == pdev->bus->number &&
+		    info->devfn == pdev->devfn) {
+			list_del(&info->link);
+			list_del(&info->global);
+			if (info->dev)
+				info->dev->dev.archdata.iommu = NULL;
+			spin_unlock_irqrestore(&device_domain_lock, flags);
+
+			iommu_detach_dev(info->bus, info->devfn);
+			free_devinfo_mem(info);
+
+			spin_lock_irqsave(&device_domain_lock, flags);
+
+			if (found)
+				break;
+			else
+				continue;
+		}
+
+		/* if there is no other devices under the same iommu
+		 * owned by this domain, clear this iommu in iommu_bmp
+		 */
+		if (device_find_matched_iommu(info->bus, info->devfn) == iommu)
+			found = 1;
+	}
+
+	if (found == 0) {
+		spin_lock_irqsave(&iommu->lock, flags);
+		clear_bit(iommu->seq_id, &domain->iommu_bmp);
+		domain->iommu_count--;
+		domain_update_iommu_coherency(domain);
+		spin_unlock_irqrestore(&iommu->lock, flags);
+	}
+
+	spin_unlock_irqrestore(&device_domain_lock, flags);
+}
+
+static void vm_domain_remove_all_dev_info(struct dmar_domain *domain)
+{
+	struct device_domain_info *info;
+	struct intel_iommu *iommu;
+	unsigned long flags;
+
+	spin_lock_irqsave(&device_domain_lock, flags);
+	while (!list_empty(&domain->devices)) {
+		info = list_entry(domain->devices.next,
+			struct device_domain_info, link);
+		list_del(&info->link);
+		list_del(&info->global);
+		if (info->dev)
+			info->dev->dev.archdata.iommu = NULL;
+
+		spin_unlock_irqrestore(&device_domain_lock, flags);
+		iommu_detach_dev(info->bus, info->devfn);
+
+		/* clear this iommu in iommu_bmp */
+		iommu = device_find_matched_iommu(info->bus, info->devfn);
+		spin_lock_irqsave(&iommu->lock, flags);
+		if (test_and_clear_bit(iommu->seq_id,
+				       &domain->iommu_bmp)) {
+			domain->iommu_count--;
+			domain_update_iommu_coherency(domain);
+		}
+		spin_unlock_irqrestore(&iommu->lock, flags);
+
+		free_devinfo_mem(info);
+		spin_lock_irqsave(&device_domain_lock, flags);
+	}
+	spin_unlock_irqrestore(&device_domain_lock, flags);
+}
+
 void intel_iommu_domain_exit(struct dmar_domain *domain)
 {
 	u64 end;
@@ -2407,7 +2561,7 @@ EXPORT_SYMBOL_GPL(intel_iommu_page_mapping);
 
 void intel_iommu_detach_dev(struct dmar_domain *domain, u8 bus, u8 devfn)
 {
-	detach_domain_for_dev(domain, bus, devfn);
+	iommu_detach_dev(bus, devfn);
 }
 EXPORT_SYMBOL_GPL(intel_iommu_detach_dev);
 
diff --git a/include/linux/dma_remapping.h b/include/linux/dma_remapping.h
index 9e39c99..f6925c5 100644
--- a/include/linux/dma_remapping.h
+++ b/include/linux/dma_remapping.h
@@ -186,6 +186,7 @@ struct dmar_domain {
 	int		flags;
 
 	int		iommu_coherency;/* iommu access is coherent or not */
+	int		iommu_count;	/* reference count of iommu */
 };
 
 /* PCI domain-device relationship */
-- 
1.5.1

Re: [PATCH 06/13] add/remove domain device info for virtual machine VT-d

[Mark McLoughlin]

On Tue, 2008-12-02 at 22:22 +0800, Han, Weidong wrote:

> Separate add/remove domain device info functions for virtual machine VT-d from natvie VT-d.
>  
> Signed-off-by: Weidong Han <weidong.han@intel.com>
> ---
>  drivers/pci/intel-iommu.c     |  164 +++++++++++++++++++++++++++++++++++++++-
>  include/linux/dma_remapping.h |    1 +
>  2 files changed, 160 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
> index 09a5150..429aff4 100644
> --- a/drivers/pci/intel-iommu.c
> +++ b/drivers/pci/intel-iommu.c
> @@ -200,6 +200,27 @@ static struct intel_iommu *domain_get_iommu(struct dmar_domain *domain)
>  	return NULL;
>  }
>  
> +static struct intel_iommu *device_find_matched_iommu(u8 bus, u8 devfn)

That's quite an unwieldy name, how about device_to_iommu() ?

> +{
> +	struct dmar_drhd_unit *drhd = NULL;
> +	int i;
> +
> +	for_each_drhd_unit(drhd) {
> +		if (drhd->ignored)
> +			continue;
> +
> +		for (i = 0; i < drhd->devices_cnt; i++)
> +			if (drhd->devices[i]->bus->number == bus &&
> +			    drhd->devices[i]->devfn == devfn)
> +				return drhd->iommu;
> +
> +		if (drhd->include_all)
> +			return drhd->iommu;
> +	}
> +
> +	return NULL;
> +}
...
> @@ -1269,9 +1292,12 @@ domain_page_mapping(struct dmar_domain *domain, dma_addr_t iova,
>  	return 0;
>  }
>  
> -static void detach_domain_for_dev(struct dmar_domain *domain, u8 bus, u8 devfn)
> +static void iommu_detach_dev(u8 bus, u8 devfn)

Would be nicer if this function took a struct intel_iommu pointer rather than bus/devfn.

>  {
> -	struct intel_iommu *iommu = domain_get_iommu(domain);
> +	struct intel_iommu *iommu = device_find_matched_iommu(bus, devfn);
> +
> +	if (!iommu)
> +		return;
>  
>  	clear_context_table(iommu, bus, devfn);
>  	iommu->flush.flush_context(iommu, 0, 0, 0,
...
> +/* "Coherency" capability may be different across iommus */
> +static void domain_update_iommu_coherency(struct dmar_domain *domain)
> +{
> +	struct dmar_drhd_unit *drhd;
> +
> +	domain->iommu_coherency = 1;
> +
> +	for_each_drhd_unit(drhd) {
> +		if (drhd->ignored)
> +			continue;
> +		if (test_bit(drhd->iommu->seq_id, &domain->iommu_bmp)) {
> +			if (!ecap_coherent(drhd->iommu->ecap)) {
> +				domain->iommu_coherency = 0;
> +				break;
> +			}
> +		}
> +	}
> +}

As I said, this belongs in the patch where you added the iommu_coherency
flag.

> +
> +static int vm_domain_add_dev_info(struct dmar_domain *domain,
> +				  struct pci_dev *pdev)
> +{
> +	struct device_domain_info *info;
> +	unsigned long flags;
> +
> +	info = alloc_devinfo_mem();
> +	if (!info)
> +		return -ENOMEM;
> +
> +	info->bus = pdev->bus->number;
> +	info->devfn = pdev->devfn;
> +	info->dev = pdev;
> +	info->domain = domain;
> +
> +	spin_lock_irqsave(&device_domain_lock, flags);
> +	list_add(&info->link, &domain->devices);
> +	list_add(&info->global, &device_domain_list);
> +	pdev->dev.archdata.iommu = info;
> +	spin_unlock_irqrestore(&device_domain_lock, flags);
> +
> +	return 0;
> +}
> +
> +static void vm_domain_remove_one_dev_info(struct dmar_domain *domain,
> +					  struct pci_dev *pdev)
> +{
> +	struct device_domain_info *info;
> +	struct intel_iommu *iommu;
> +	unsigned long flags;
> +	int found = 0;
> +
> +	iommu = device_find_matched_iommu(pdev->bus->number, pdev->devfn);
> +
> +	spin_lock_irqsave(&device_domain_lock, flags);
> +	while (!list_empty(&domain->devices)) {
> +		info = list_entry(domain->devices.next,
> +			struct device_domain_info, link);
> +		if (info->bus == pdev->bus->number &&
> +		    info->devfn == pdev->devfn) {
> +			list_del(&info->link);
> +			list_del(&info->global);
> +			if (info->dev)
> +				info->dev->dev.archdata.iommu = NULL;
> +			spin_unlock_irqrestore(&device_domain_lock, flags);
> +
> +			iommu_detach_dev(info->bus, info->devfn);
> +			free_devinfo_mem(info);
> +
> +			spin_lock_irqsave(&device_domain_lock, flags);
> +
> +			if (found)
> +				break;
> +			else
> +				continue;
> +		}
> +
> +		/* if there is no other devices under the same iommu
> +		 * owned by this domain, clear this iommu in iommu_bmp
> +		 */
> +		if (device_find_matched_iommu(info->bus, info->devfn) == iommu)
> +			found = 1;
> +	}
> +
> +	if (found == 0) {
> +		spin_lock_irqsave(&iommu->lock, flags);
> +		clear_bit(iommu->seq_id, &domain->iommu_bmp);
> +		domain->iommu_count--;
> +		domain_update_iommu_coherency(domain);
> +		spin_unlock_irqrestore(&iommu->lock, flags);
> +	}
> +
> +	spin_unlock_irqrestore(&device_domain_lock, flags);
> +}
> +
> +static void vm_domain_remove_all_dev_info(struct dmar_domain *domain)
> +{
> +	struct device_domain_info *info;
> +	struct intel_iommu *iommu;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&device_domain_lock, flags);
> +	while (!list_empty(&domain->devices)) {
> +		info = list_entry(domain->devices.next,
> +			struct device_domain_info, link);
> +		list_del(&info->link);
> +		list_del(&info->global);
> +		if (info->dev)
> +			info->dev->dev.archdata.iommu = NULL;
> +
> +		spin_unlock_irqrestore(&device_domain_lock, flags);
> +		iommu_detach_dev(info->bus, info->devfn);
> +
> +		/* clear this iommu in iommu_bmp */
> +		iommu = device_find_matched_iommu(info->bus, info->devfn);
> +		spin_lock_irqsave(&iommu->lock, flags);
> +		if (test_and_clear_bit(iommu->seq_id,
> +				       &domain->iommu_bmp)) {
> +			domain->iommu_count--;
> +			domain_update_iommu_coherency(domain);
> +		}
> +		spin_unlock_irqrestore(&iommu->lock, flags);
> +
> +		free_devinfo_mem(info);
> +		spin_lock_irqsave(&device_domain_lock, flags);
> +	}
> +	spin_unlock_irqrestore(&device_domain_lock, flags);
> +}

You're adding three functions here which are essentially copies of code
that already exists, with some minor changes. You need to refactor the
existing code and modify it to handle these cases.

Cheers,
Mark.

RE: [PATCH 06/13] add/remove domain device info for virtual machine VT-d

[Han, Weidong]

Mark McLoughlin wrote:
> On Tue, 2008-12-02 at 22:22 +0800, Han, Weidong wrote:
> 
>> Separate add/remove domain device info functions for virtual machine
>> VT-d from natvie VT-d. 
>> 
>> Signed-off-by: Weidong Han <weidong.han@intel.com>
>> ---
>>  drivers/pci/intel-iommu.c     |  164
>>  +++++++++++++++++++++++++++++++++++++++-
>>  include/linux/dma_remapping.h |    1 + 2 files changed, 160
>> insertions(+), 5 deletions(-) 
>> 
>> diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
>> index 09a5150..429aff4 100644
>> --- a/drivers/pci/intel-iommu.c
>> +++ b/drivers/pci/intel-iommu.c
>> @@ -200,6 +200,27 @@ static struct intel_iommu
>>  *domain_get_iommu(struct dmar_domain *domain)  	return NULL; }
>> 
>> +static struct intel_iommu *device_find_matched_iommu(u8 bus, u8
>> devfn) 
> 
> That's quite an unwieldy name, how about device_to_iommu() ?

will rename it.

> 
>> +{
>> +	struct dmar_drhd_unit *drhd = NULL;
>> +	int i;
>> +
>> +	for_each_drhd_unit(drhd) {
>> +		if (drhd->ignored)
>> +			continue;
>> +
>> +		for (i = 0; i < drhd->devices_cnt; i++)
>> +			if (drhd->devices[i]->bus->number == bus &&
>> +			    drhd->devices[i]->devfn == devfn)
>> +				return drhd->iommu;
>> +
>> +		if (drhd->include_all)
>> +			return drhd->iommu;
>> +	}
>> +
>> +	return NULL;
>> +}
> ...
>> @@ -1269,9 +1292,12 @@ domain_page_mapping(struct dmar_domain
>>  *domain, dma_addr_t iova,  	return 0; }
>> 
>> -static void detach_domain_for_dev(struct dmar_domain *domain, u8
>> bus, u8 devfn) +static void iommu_detach_dev(u8 bus, u8 devfn)
> 
> Would be nicer if this function took a struct intel_iommu pointer
> rather than bus/devfn. 

intel_iommu can be got by bus and devfn, so I didn't add the parameter. But your suggestion is reasonable. 

> 
>>  {
>> -	struct intel_iommu *iommu = domain_get_iommu(domain);
>> +	struct intel_iommu *iommu = device_find_matched_iommu(bus, devfn);
>> + +	if (!iommu)
>> +		return;
>> 
>>  	clear_context_table(iommu, bus, devfn);
>>  	iommu->flush.flush_context(iommu, 0, 0, 0, ...
>> +/* "Coherency" capability may be different across iommus */
>> +static void domain_update_iommu_coherency(struct dmar_domain
>> *domain) +{ +	struct dmar_drhd_unit *drhd;
>> +
>> +	domain->iommu_coherency = 1;
>> +
>> +	for_each_drhd_unit(drhd) {
>> +		if (drhd->ignored)
>> +			continue;
>> +		if (test_bit(drhd->iommu->seq_id, &domain->iommu_bmp)) {
>> +			if (!ecap_coherent(drhd->iommu->ecap)) {
>> +				domain->iommu_coherency = 0;
>> +				break;
>> +			}
>> +		}
>> +	}
>> +}
> 
> As I said, this belongs in the patch where you added the
> iommu_coherency 
> flag.
> 
>> +
>> +static int vm_domain_add_dev_info(struct dmar_domain *domain, +				
>> struct pci_dev *pdev) +{
>> +	struct device_domain_info *info;
>> +	unsigned long flags;
>> +
>> +	info = alloc_devinfo_mem();
>> +	if (!info)
>> +		return -ENOMEM;
>> +
>> +	info->bus = pdev->bus->number;
>> +	info->devfn = pdev->devfn;
>> +	info->dev = pdev;
>> +	info->domain = domain;
>> +
>> +	spin_lock_irqsave(&device_domain_lock, flags);
>> +	list_add(&info->link, &domain->devices);
>> +	list_add(&info->global, &device_domain_list);
>> +	pdev->dev.archdata.iommu = info;
>> +	spin_unlock_irqrestore(&device_domain_lock, flags); +
>> +	return 0;
>> +}
>> +
>> +static void vm_domain_remove_one_dev_info(struct dmar_domain
>> *domain, +					  struct pci_dev *pdev) +{
>> +	struct device_domain_info *info;
>> +	struct intel_iommu *iommu;
>> +	unsigned long flags;
>> +	int found = 0;
>> +
>> +	iommu = device_find_matched_iommu(pdev->bus->number, pdev->devfn);
>> + +	spin_lock_irqsave(&device_domain_lock, flags);
>> +	while (!list_empty(&domain->devices)) {
>> +		info = list_entry(domain->devices.next,
>> +			struct device_domain_info, link);
>> +		if (info->bus == pdev->bus->number &&
>> +		    info->devfn == pdev->devfn) {
>> +			list_del(&info->link);
>> +			list_del(&info->global);
>> +			if (info->dev)
>> +				info->dev->dev.archdata.iommu = NULL;
>> +			spin_unlock_irqrestore(&device_domain_lock, flags); +
>> +			iommu_detach_dev(info->bus, info->devfn);
>> +			free_devinfo_mem(info);
>> +
>> +			spin_lock_irqsave(&device_domain_lock, flags);
>> +
>> +			if (found)
>> +				break;
>> +			else
>> +				continue;
>> +		}
>> +
>> +		/* if there is no other devices under the same iommu
>> +		 * owned by this domain, clear this iommu in iommu_bmp +		 */
>> +		if (device_find_matched_iommu(info->bus, info->devfn) == iommu)
>> +			found = 1; +	}
>> +
>> +	if (found == 0) {
>> +		spin_lock_irqsave(&iommu->lock, flags);
>> +		clear_bit(iommu->seq_id, &domain->iommu_bmp);
>> +		domain->iommu_count--;
>> +		domain_update_iommu_coherency(domain);
>> +		spin_unlock_irqrestore(&iommu->lock, flags);
>> +	}
>> +
>> +	spin_unlock_irqrestore(&device_domain_lock, flags); +}
>> +
>> +static void vm_domain_remove_all_dev_info(struct dmar_domain
>> *domain) +{ +	struct device_domain_info *info;
>> +	struct intel_iommu *iommu;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&device_domain_lock, flags);
>> +	while (!list_empty(&domain->devices)) {
>> +		info = list_entry(domain->devices.next,
>> +			struct device_domain_info, link);
>> +		list_del(&info->link);
>> +		list_del(&info->global);
>> +		if (info->dev)
>> +			info->dev->dev.archdata.iommu = NULL;
>> +
>> +		spin_unlock_irqrestore(&device_domain_lock, flags);
>> +		iommu_detach_dev(info->bus, info->devfn);
>> +
>> +		/* clear this iommu in iommu_bmp */
>> +		iommu = device_find_matched_iommu(info->bus, info->devfn);
>> +		spin_lock_irqsave(&iommu->lock, flags);
>> +		if (test_and_clear_bit(iommu->seq_id,
>> +				       &domain->iommu_bmp)) {
>> +			domain->iommu_count--;
>> +			domain_update_iommu_coherency(domain);
>> +		}
>> +		spin_unlock_irqrestore(&iommu->lock, flags);
>> +
>> +		free_devinfo_mem(info);
>> +		spin_lock_irqsave(&device_domain_lock, flags);
>> +	}
>> +	spin_unlock_irqrestore(&device_domain_lock, flags);
>> +}
> 
> You're adding three functions here which are essentially copies of
> code 
> that already exists, with some minor changes. You need to refactor the
> existing code and modify it to handle these cases.

Adding these functions is to remove domain flag judgment in many functions. domain flag judgment seems brittle. So it's a balance. I will clean up it again.

Regards,
Weidong


> 
> Cheers,
> Mark.