Re: [PATCH 2/2] IOMMU Identity Mapping Support: Intel IOMMU implementation

[Chris Wright <chrisw@sous-sol.org>]

* Fenghua Yu (fenghua.yu@intel.com) wrote:
> IOMMU Identity Mapping Support: Intel IOMMU implementation

BTW, this doesn't apply at all to Linus' current tree, and does w/ minor
fuzz and offset to David's VT-d tree.

Some quick feedback.

> @@ -1189,48 +1207,74 @@ void free_dmar_iommu(struct intel_iommu *iommu)
>  	free_context_table(iommu);
>  }
>  
> -static struct dmar_domain * iommu_alloc_domain(struct intel_iommu *iommu)
> +/* Sequential domain id starting from 0. */
> +static unsigned long domain_id;

This doesn't look SMP safe.

> +static int iommu_prepare_static_identity_mapping(void)
> +{
> +	int i;
> +	struct pci_dev *pdev = NULL;
> +	int ret;
> +
> +	ret = si_domain_init();
> +	if (ret)
> +		return -EFAULT;
> +
> +	printk(KERN_INFO "IOMMU: Setting identity map:\n");
> +	for_each_pci_dev(pdev) {
> +		/* Devices not in the identity list won't do identity map. */
> +		if (!identity_list(pdev))
> +			continue;
> +
> +		for (i = 0; i < e820.nr_map; i++) {
> +			struct e820entry *ei = &e820.map[i];
> +
> +			if (ei->type == E820_RAM) {
> +				ret = iommu_prepare_identity_map(pdev,

What about RMRR?

And in this mode,  you shouldn't need gfx workaround.

> +					ei->addr, ei->addr + ei->size);
> +				if (ret)  {
> +					printk(KERN_INFO "1:1 mapping to one domain failed.\n");
> +					return -EFAULT;
> +				}
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +int __init init_dmars(void)
>  {
>  	struct dmar_drhd_unit *drhd;
>  	struct dmar_rmrr_unit *rmrr;
> @@ -2076,6 +2206,7 @@ static int __init init_dmars(void)
>  		}
>  	}
>  
> +
>  	/*
>  	 * If pass through is set and enabled, context entries of all pci
>  	 * devices are intialized by pass through translation type.
> @@ -2093,6 +2224,9 @@ static int __init init_dmars(void)
>  	 * identity mappings for rmrr, gfx, and isa.
>  	 */
>  	if (!iommu_pass_through) {
> +		if (iommu_identity_mapping)
> +			iommu_prepare_static_identity_mapping();
> +
>  		/*
>  		 * For each rmrr
>  		 *   for each dev attached to rmrr
> @@ -2107,6 +2241,7 @@ static int __init init_dmars(void)
>  		 *    endfor
>  		 * endfor
>  		 */

...Ah, RMRR looks OK.

> +		printk(KERN_INFO "IOMMU: Setting RMRR:\n");
>  		for_each_rmrr_units(rmrr) {
>  			for (i = 0; i < rmrr->devices_cnt; i++) {
>  				pdev = rmrr->devices[i];
> @@ -2259,6 +2394,9 @@ static dma_addr_t __intel_map_single(struct device *hwdev, phys_addr_t paddr,
>  	int ret;
>  	struct intel_iommu *iommu;
>  
> +	if (identity_list(pdev))
> +		return paddr;
> +

This is same as DUMMY_DEVICE_DOMAIN_INFO.  Please consolidate to a test
that just says "do i need translation".

And what about DMA mask smaller than physical memory.  The PT mode drops
back to swiotlb iirc.


>  	BUG_ON(dir == DMA_NONE);
>  	if (pdev->dev.archdata.iommu == DUMMY_DEVICE_DOMAIN_INFO)
>  		return paddr;
> @@ -2401,6 +2539,9 @@ static void intel_unmap_page(struct device *dev, dma_addr_t dev_addr,
>  	struct iova *iova;
>  	struct intel_iommu *iommu;
>  
> +	if (identity_list(pdev))
> +		return;
> +

Same...duplicate test

>  	if (pdev->dev.archdata.iommu == DUMMY_DEVICE_DOMAIN_INFO)
>  		return;
>  	domain = find_domain(pdev);
> @@ -2492,6 +2633,9 @@ static void intel_unmap_sg(struct device *hwdev, struct scatterlist *sglist,
>  	struct scatterlist *sg;
>  	struct intel_iommu *iommu;
>  
> +	if (identity_list(pdev))
> +		return;
> +

Same duplicate test

>  	if (pdev->dev.archdata.iommu == DUMMY_DEVICE_DOMAIN_INFO)
>  		return;
>  
> @@ -2552,6 +2696,16 @@ static int intel_map_sg(struct device *hwdev, struct scatterlist *sglist, int ne
>  	unsigned long start_addr;
>  	struct intel_iommu *iommu;
>  
> +	if (identity_list(pdev)) {
> +		for_each_sg(sglist, sg, nelems, i) {
> +			addr = page_to_phys(sg_page(sg)) + sg->offset;
> +			sg->dma_address = addr;
> +			sg->dma_length = sg->length;
> +		}
> +

This is just the same as intel_nontranslate_map_sg()
Please consolidate this.

thanks,
-chris