Re: [PATCH 2/4][VTD] modifications to intel-iommu.c.

[Muli Ben-Yehuda <muli@il.ibm.com>]

On Mon, Jun 09, 2008 at 05:41:29PM -0700, Kay, Allen M wrote:

> Modification to intel-iommu.c to support vt-d page table and context
> table mapping in kvm.  Mods to dmar.c and iova.c are due to header
> file moves to include/linux.

Hi Allen,

Minor comments below, patches to follow up.

> Signed-off-by: Allen M. Kay <allen.m.kay@intel.com>
> 
> diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
> index 66c0fd2..a694e62 100644
> --- a/drivers/pci/intel-iommu.c
> +++ b/drivers/pci/intel-iommu.c
> @@ -20,6 +20,7 @@
>   * Author: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
>   */
>  
> +#undef DEBUG

Nit: this should be submitted upstream seperately.

>  #include <linux/init.h>
>  #include <linux/bitmap.h>
>  #include <linux/debugfs.h>
> @@ -33,8 +34,8 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/mempool.h>
>  #include <linux/timer.h>
> -#include "iova.h"
> -#include "intel-iommu.h"
> +#include <linux/iova.h>
> +#include <linux/intel-iommu.h>
>  #include <asm/proto.h> /* force_iommu in this header in x86-64*/
>  #include <asm/cacheflush.h>
>  #include <asm/gart.h>
> @@ -160,7 +161,7 @@ static inline void *alloc_domain_mem(void)
>  	return iommu_kmem_cache_alloc(iommu_domain_cache);
>  }
>  
> -static inline void free_domain_mem(void *vaddr)
> +static void free_domain_mem(void *vaddr)
>  {
>  	kmem_cache_free(iommu_domain_cache, vaddr);
>  }
> @@ -1263,11 +1264,8 @@ static int domain_context_mapping_one(struct dmar_domain *domain,
>  	if (!context)
>  		return -ENOMEM;
>  	spin_lock_irqsave(&iommu->lock, flags);
> -	if (context_present(*context)) {
> -		spin_unlock_irqrestore(&iommu->lock, flags);
> -		return 0;
> -	}
>  
> +	context_clear_entry(*context);
>  	context_set_domain_id(*context, domain->id);
>  	context_set_address_width(*context, domain->agaw);
>  	context_set_address_root(*context, virt_to_phys(domain->pgd));
> @@ -1414,7 +1412,7 @@ static void domain_remove_dev_info(struct dmar_domain *domain)
>   * find_domain
>   * Note: we use struct pci_dev->dev.archdata.iommu stores the info
>   */
> -struct dmar_domain *
> +static struct dmar_domain *
>  find_domain(struct pci_dev *pdev)
>  {
>  	struct device_domain_info *info;
> @@ -2408,3 +2406,96 @@ int __init intel_iommu_init(void)
>  	return 0;
>  }
>  
> +void kvm_intel_iommu_domain_exit(struct dmar_domain *domain)
> +{
> +	u64 end;
> +
> +	/* Domain 0 is reserved, so dont process it */
> +	if (!domain)
> +		return;
> +
> +	end = DOMAIN_MAX_ADDR(domain->gaw);
> +	end = end & (~PAGE_MASK_4K);
> +
> +	/* clear ptes */
> +	dma_pte_clear_range(domain, 0, end);
> +
> +	/* free page tables */
> +	dma_pte_free_pagetable(domain, 0, end);
> +
> +	iommu_free_domain(domain);
> +	free_domain_mem(domain);
> +}
> +EXPORT_SYMBOL_GPL(kvm_intel_iommu_domain_exit);
> +
> +struct dmar_domain *kvm_intel_iommu_domain_alloc(struct pci_dev *pdev)
> +{
> +	struct dmar_drhd_unit *drhd;
> +	struct dmar_domain *domain;
> +	struct intel_iommu *iommu;
> +
> +	drhd = dmar_find_matched_drhd_unit(pdev);
> +	if (!drhd) {
> +		printk(KERN_ERR "kvm_intel_iommu_domain_alloc: drhd == NULL\n");
> +		return NULL;
> +	}
> +
> +	iommu = drhd->iommu;
> +	if (!iommu) {
> +		printk(KERN_ERR
> +			"kvm_intel_iommu_domain_alloc: iommu == NULL\n");
> +		return NULL;
> +	}
> +	domain = iommu_alloc_domain(iommu);
> +	if (!domain) {
> +		printk(KERN_ERR
> +			"kvm_intel_iommu_domain_alloc: domain == NULL\n");
> +		return NULL;
> +	}
> +	if (domain_init(domain, DEFAULT_DOMAIN_ADDRESS_WIDTH)) {
> +		printk(KERN_ERR
> +			"kvm_intel_iommu_domain_alloc: domain_init() failed\n");
> +		kvm_intel_iommu_domain_exit(domain);
> +		return NULL;
> +	}
> +	return domain;
> +}
> +EXPORT_SYMBOL_GPL(kvm_intel_iommu_domain_alloc);
> +
> +int kvm_intel_iommu_context_mapping(
> +	struct dmar_domain *domain, struct pci_dev *pdev)
> +{
> +	int rc;
> +	rc = domain_context_mapping(domain, pdev);
> +	return rc;
> +}

Unnecessary wrapper.

> +EXPORT_SYMBOL_GPL(kvm_intel_iommu_context_mapping);
> +
> +int kvm_intel_iommu_page_mapping(
> +	struct dmar_domain *domain, dma_addr_t iova,
> +	u64 hpa, size_t size, int prot)
> +{
> +	int rc;
> +	rc = domain_page_mapping(domain, iova, hpa, size, prot);
> +	return rc;
> +}

Unnecessary wrapper.

> +EXPORT_SYMBOL_GPL(kvm_intel_iommu_page_mapping);
> +
> +void kvm_intel_iommu_detach_dev(struct dmar_domain *domain, u8 bus, u8 devfn)
> +{
> +	kvm_intel_iommu_detach_dev(domain, bus, devfn);
> +}

This doesn't look correct--we should probably tear down the domain's
page tables and context here.

> +EXPORT_SYMBOL_GPL(kvm_intel_iommu_detach_dev);
> +
> +struct dmar_domain *
> +kvm_intel_iommu_find_domain(struct pci_dev *pdev)
> +{
> +	return find_domain(pdev);
> +}

Unnecessary wrapper.

Cheers,
Muli