Re: [PATCH v3 6/6] VT-d: support the device IOTLB

[Yu Zhao <yu.zhao@intel.com>]

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