On Sun, Feb 15, 2009 at 07:20:52AM +0800, Grant Grundler wrote: > On Thu, Feb 12, 2009 at 08:50:38PM +0800, Yu Zhao wrote: > > Support device IOTLB (i.e. ATS) for both native and KVM environments. > > + > > +static void iommu_enable_dev_iotlb(struct device_domain_info *info) > > +{ > > + pci_enable_ats(info->dev, VTD_PAGE_SHIFT); > > +} > > Why is a static function defined that calls a global function? There would be some extra steps to do before VT-d enables ATS in the future, so this wrapper makes code expandable later. > > > + > > +static void iommu_disable_dev_iotlb(struct device_domain_info *info) > > +{ > > + if (info->dev && pci_ats_enabled(info->dev)) > > + pci_disable_ats(info->dev); > > +} > > ditto. pci_disable_ats() should be able to handle the case when > "info->dev" is NULL and will know if ATS is enabled. The info->dev could be NULL only because the VT-d code makes it so. AMD an IBM IOMMU may not have this requirement. If we make pci_disable_ats() accept NULL pci_dev, it would fail to catch some errors like using pci_disable_ats without calling pci_enable_ats before. > > I think both of these functions can be dropped and just directly call pci_*_ats(). > > > + > > +static void iommu_flush_dev_iotlb(struct dmar_domain *domain, > > + u64 addr, unsigned mask) > > +{ > > + int rc; > > + u16 sid, qdep; > > + unsigned long flags; > > + struct device_domain_info *info; > > + > > + spin_lock_irqsave(&device_domain_lock, flags); > > + list_for_each_entry(info, &domain->devices, link) { > > Would it be possible to define a single domain for each PCI device? > Or does "domain" represent an IOMMU? > Sorry, I forgot...I'm sure someone has mentioned this the past. A domain represents one translation mapping. For device used by the host, there is one domain per device. Device assigned to a guest shares one domain. > > I want to point out list_for_each_entry() is effectively a "nested loop". > iommu_flush_dev_iotlb() will get called alot from flush_unmaps(). > Perhaps do the lookup once there and pass that as a parameter? > I don't know if that is feasible. But if this is a very frequently > used code path, every CPU cycle counts. iommu_flush_dev_iotlb() is only used to flush the devices used in the host, which means there is always one entry on the list. > > > > + if (!info->dev || !pci_ats_enabled(info->dev)) > > + continue; > > + > > + sid = info->bus << 8 | info->devfn; > > + qdep = pci_ats_queue_depth(info->dev); > > re Matthew Wilcox's comment - looks like caching ats_queue_depth > is appropriate. Yes, it's cached as of v3. > > + rc = qi_flush_dev_iotlb(info->iommu, sid, qdep, addr, mask); > > + if (rc) > > + printk(KERN_ERR "IOMMU: flush device IOTLB failed\n"); > > Can this be a dev_printk please? Yes, will replace it with dev_err(). > Perhaps in general review the use of "printk" so when errors are reported, > users will know which devices might be affected by the failure. > If more than a few printk's should be "converted" to dev_printk(), I'd > be happy if that were a seperate patch (submitted later). > > > > pr_debug("Set context mapping for %02x:%02x.%d\n", > > bus, PCI_SLOT(devfn), PCI_FUNC(devfn)); > > @@ -1534,7 +1608,11 @@ static int domain_context_mapping_one(struct dmar_domain *domain, > > context_set_domain_id(context, id); > > context_set_address_width(context, iommu->agaw); > > context_set_address_root(context, virt_to_phys(pgd)); > > - context_set_translation_type(context, CONTEXT_TT_MULTI_LEVEL); > > + info = iommu_support_dev_iotlb(domain, bus, devfn); > > + if (info) > > + context_set_translation_type(context, CONTEXT_TT_DEV_IOTLB); > > + else > > + context_set_translation_type(context, CONTEXT_TT_MULTI_LEVEL); > > Would it be ok to rewrite this as: > + context_set_translation_type(context, > + info ? CONTEXT_TT_DEV_IOTLB : CONTEXT_TT_MULTI_LEVEL); Yes, this one looks better. > > > context_set_fault_enable(context); > > context_set_present(context); > > domain_flush_cache(domain, context, sizeof(*context)); > > @@ -1546,6 +1624,8 @@ static int domain_context_mapping_one(struct dmar_domain *domain, > > iommu_flush_write_buffer(iommu); > > else > > iommu->flush.flush_iotlb(iommu, 0, 0, 0, DMA_TLB_DSI_FLUSH, 0); > > Adding a blank line here would make this more readable. > (AFAIK, not required by coding style, just my opinion.) Yes, I prefer a bank line here too, somehow I missed it. > > > + if (info) > > + iommu_enable_dev_iotlb(info); > > Could iommu_enable_dev_iotlb() (or pci_enable_ats()) check if "info" is NULL? > Then this would just be a simple function call. > > And it would be consistent with usage of iommu_disable_dev_iotlb(). Yes, good idea. Thanks a lot for reviewing it! Yu -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html