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

[Mark McLoughlin <markmc@redhat.com>]

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.